* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-02-06 20:22 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190204081947.25152-3-ronald@innovation.ch>
On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> The keyboard and trackpad on recent MacBook's (since 8,1) and
> MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> of USB, as previously. The higher level protocol is not publicly
> documented and hence has been reverse engineered. As a consequence there
> are still a number of unknown fields and commands. However, the known
> parts have been working well and received extensive testing and use.
>
> In order for this driver to work, the proper SPI drivers need to be
> loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> reason enabling this driver in the config implies enabling the above
> drivers.
Thanks for doing this. My review below.
> +/**
I'm not sure it's kernel doc format comment.
> + * The keyboard and touchpad controller on the MacBookAir6, MacBookPro12,
> + * MacBook8 and newer can be driven either by USB or SPI. However the USB
> + * pins are only connected on the MacBookAir6 and 7 and the MacBookPro12.
> + * All others need this driver. The interface is selected using ACPI methods:
> + *
> + * * UIEN ("USB Interface Enable"): If invoked with argument 1, disables SPI
> + * and enables USB. If invoked with argument 0, disables USB.
> + * * UIST ("USB Interface Status"): Returns 1 if USB is enabled, 0 otherwise.
> + * * SIEN ("SPI Interface Enable"): If invoked with argument 1, disables USB
> + * and enables SPI. If invoked with argument 0, disables SPI.
> + * * SIST ("SPI Interface Status"): Returns 1 if SPI is enabled, 0 otherwise.
> + * * ISOL: Resets the four GPIO pins used for SPI. Intended to be invoked with
> + * argument 1, then once more with argument 0.
> + *
> + * UIEN and UIST are only provided on models where the USB pins are connected.
> + *
> + * SPI-based Protocol
> + * ------------------
> + *
> + * The device and driver exchange messages (struct message); each message is
> + * encapsulated in one or more packets (struct spi_packet). There are two types
> + * of exchanges: reads, and writes. A read is signaled by a GPE, upon which one
> + * message can be read from the device. A write exchange consists of writing a
> + * command message, immediately reading a short status packet, and then, upon
> + * receiving a GPE, reading the response message. Write exchanges cannot be
> + * interleaved, i.e. a new write exchange must not be started till the previous
> + * write exchange is complete. Whether a received message is part of a read or
> + * write exchange is indicated in the encapsulating packet's flags field.
> + *
> + * A single message may be too large to fit in a single packet (which has a
> + * fixed, 256-byte size). In that case it will be split over multiple,
> + * consecutive packets.
> + */
> +
> +#define pr_fmt(fmt) "applespi: " fmt
Better to use usual pattern.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/property.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/crc16.h>
> +#include <linux/wait.h>
> +#include <linux/leds.h>
> +#include <linux/ktime.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input-polldev.h>
> +#include <linux/workqueue.h>
> +#include <linux/efi.h>
Can we keep them sorted? Do you need, btw, all of them?
+ blank line.
> +#include <asm/barrier.h>
> +#define MIN_KBD_BL_LEVEL 32
> +#define MAX_KBD_BL_LEVEL 255
I would rather use similar pattern as below, i.e. KBD_..._MIN/_MAX.
> +#define KBD_BL_LEVEL_SCALE 1000000
> +#define KBD_BL_LEVEL_ADJ \
> + ((MAX_KBD_BL_LEVEL - MIN_KBD_BL_LEVEL) * KBD_BL_LEVEL_SCALE / 255)
> +#define debug_print(mask, fmt, ...) \
> + do { \
> + if (debug & mask) \
> + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> + } while (0)
> +
> +#define debug_print_header(mask) \
> + debug_print(mask, "--- %s ---------------------------\n", \
> + applespi_debug_facility(mask))
> +
> +#define debug_print_buffer(mask, fmt, ...) \
> + do { \
> + if (debug & mask) \
> + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> + false); \
> + } while (0)
I'm not sure we need all of these... But okay, the driver is
reverse-engineered, perhaps we can drop it later on.
> +#define SPI_RW_CHG_DLY 100 /* from experimentation, in us */
In _US would be good to have in a constant name, i.e.
SPI_RW_CHG_DELAY_US
> +static unsigned int fnmode = 1;
> +module_param(fnmode, uint, 0644);
> +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, [1] = fkeyslast, 2 = fkeysfirst)");
fn -> Fn ?
Ditto for the rest.
Btw, do we need all these parameters? Can't we do modify behaviour run-time
using some Input framework facilities?
> +static unsigned int iso_layout;
> +module_param(iso_layout, uint, 0644);
> +MODULE_PARM_DESC(iso_layout, "Enable/Disable hardcoded ISO-layout of the keyboard. ([0] = disabled, 1 = enabled)");
bool ?
> +static int touchpad_dimensions[4];
> +module_param_array(touchpad_dimensions, int, NULL, 0444);
> +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as x_min,x_max,y_min,y_max .");
We have some parsers for similar data as in format
%dx%d%+d%+d ?
For example, 200x100+20+10.
(But still same question, wouldn't input subsystem allows to do this run-time?)
> +/**
> + * struct keyboard_protocol - keyboard message.
> + * message.type = 0x0110, message.length = 0x000a
> + *
> + * @unknown1: unknown
> + * @modifiers: bit-set of modifier/control keys pressed
> + * @unknown2: unknown
> + * @keys_pressed: the (non-modifier) keys currently pressed
> + * @fn_pressed: whether the fn key is currently pressed
> + * @crc_16: crc over the whole message struct (message header +
> + * this struct) minus this @crc_16 field
Something wrong with indentation. Check it over all your kernel doc comments.
> + */
> +struct touchpad_info_protocol {
> + __u8 unknown1[105];
> + __le16 model_id;
> + __u8 unknown2[3];
> + __le16 crc_16;
> +} __packed;
112 % 16 == 0, why do you need __packed?
> + __le16 crc_16;
Can't you use crc16 everywhere for the name?
> +struct applespi_tp_info {
> + int x_min;
> + int x_max;
> + int y_min;
> + int y_max;
> +};
Perhaps use the same format as in struct drm_rect in order to possibility of
unifying them in the future?
> + { },
Terminators are better without comma.
> + switch (log_mask) {
> + case DBG_CMD_TP_INI:
> + return "Touchpad Initialization";
> + case DBG_CMD_BL:
> + return "Backlight Command";
> + case DBG_CMD_CL:
> + return "Caps-Lock Command";
> + case DBG_RD_KEYB:
> + return "Keyboard Event";
> + case DBG_RD_TPAD:
> + return "Touchpad Event";
> + case DBG_RD_UNKN:
> + return "Unknown Event";
> + case DBG_RD_IRQ:
> + return "Interrupt Request";
> + case DBG_RD_CRC:
> + return "Corrupted packet";
> + case DBG_TP_DIM:
> + return "Touchpad Dimensions";
> + default:
> + return "-Unknown-";
What the difference to RD_UNKN ?
Perhaps "Unrecognized ... "-ish better?
> + }
> +static inline bool applespi_check_write_status(struct applespi_data *applespi,
> + int sts)
Indentation broken.
> +{
> + static u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 };
Spell it fully, i.e. status_ok.
> + bool ret = true;
Directly return from each branch, it also will make 'else' redundant.
> +
> + if (sts < 0) {
> + ret = false;
> + pr_warn("Error writing to device: %d\n", sts);
> + } else if (memcmp(applespi->tx_status, sts_ok,
> + APPLESPI_STATUS_SIZE) != 0) {
Redundant ' != 0' part.
After removing this and 'else' would be fit on one line.
> + ret = false;
> + pr_warn("Error writing to device: %x %x %x %x\n",
> + applespi->tx_status[0], applespi->tx_status[1],
> + applespi->tx_status[2], applespi->tx_status[3]);
pr_warn("...: %ph\n", applespi->tx_status);
> + }
> +
> + return ret;
> +}
> +static int applespi_enable_spi(struct applespi_data *applespi)
> +{
> + int result;
Sometimes you have ret, sometimes this. Better to unify across the code.
> + unsigned long long spi_status;
> + return 0;
> +}
> +static void applespi_async_write_complete(void *context)
> +{
> + struct applespi_data *applespi = context;
> + if (!applespi_check_write_status(applespi, applespi->wr_m.status))
> + /*
> + * If we got an error, we presumably won't get the expected
> + * response message either.
> + */
> + applespi_msg_complete(applespi, true, false);
Style issue, either use {} or positive condition like
if (...)
return;
...
> +}
> +static int applespi_send_cmd_msg(struct applespi_data *applespi)
> +{
> + if (applespi->want_tp_info_cmd) {
> + } else if (applespi->want_mt_init_cmd) {
> + } else if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
> + } else if (applespi->want_bl_level != applespi->have_bl_level) {
> + } else {
> + return 0;
> + }
Can we refactor to use some kind of enumeration and do switch-case here?
> + message->counter = applespi->cmd_msg_cntr++ & 0xff;
Usual pattern for this kind of entries is
x = ... % 256;
Btw, isn't 256 is defined somewhere?
> + crc = crc16(0, (u8 *)message, le16_to_cpu(packet->length) - 2);
> + *((__le16 *)&message->data[msg_len - 2]) = cpu_to_le16(crc);
put_unaligned_le16() ?
> + if (sts != 0) {
> + pr_warn("Error queueing async write to device: %d\n", sts);
> + } else {
> + applespi->cmd_msg_queued = true;
> + applespi->write_active = true;
> + }
Usual pattern is
if (sts) {
...
return sts;
}
...
return 0;
Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where
appropriate acpi_handle_warn(), etc.
> +
> + return sts;
> +}
> +static void applespi_init(struct applespi_data *applespi, bool is_resume)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (!is_resume)
> + applespi->want_tp_info_cmd = true;
> + else
> + applespi->want_mt_init_cmd = true;
Why not positive conditional?
> + applespi_send_cmd_msg(applespi);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
> +static void applespi_set_bl_level(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct applespi_data *applespi =
> + container_of(led_cdev, struct applespi_data, backlight_info);
> + unsigned long flags;
> + int sts;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (value == 0)
> + applespi->want_bl_level = value;
> + else
> + /*
> + * The backlight does not turn on till level 32, so we scale
> + * the range here so that from a user's perspective it turns
> + * on at 1.
> + */
> + applespi->want_bl_level = (unsigned int)
> + ((value * KBD_BL_LEVEL_ADJ) / KBD_BL_LEVEL_SCALE +
> + MIN_KBD_BL_LEVEL);
Why do you need casting? Your defines better to have U or UL suffixes where
appropriate.
Besides, run checkpatch.pl!
> +
> + sts = applespi_send_cmd_msg(applespi);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
> +static int applespi_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct applespi_data *applespi = input_get_drvdata(dev);
> +
> + switch (type) {
> + case EV_LED:
> + applespi_set_capsl_led(applespi,
> + !!test_bit(LED_CAPSL, dev->led));
I would put it on one line disregard checkpatch warnings.
> + return 0;
> + }
> +
> + return -1;
Can't you use appropriate Linux error code?
> +}
> +/* lifted from the BCM5974 driver */
> +/* convert 16-bit little endian to signed integer */
> +static inline int raw2int(__le16 x)
> +{
> + return (signed short)le16_to_cpu(x);
> +}
Perhaps it's time to introduce it inside include/linux/input.h ?
> +static void report_tp_state(struct applespi_data *applespi,
> + struct touchpad_protocol *t)
> +{
> + static int min_x, max_x, min_y, max_y;
> + static bool dim_updated;
> + static ktime_t last_print;
> +
Redundant.
> + const struct tp_finger *f;
> + struct input_dev *input;
> + const struct applespi_tp_info *tp_info = &applespi->tp_info;
> + int i, n;
> +
> + /* touchpad_input_dev is set async in worker */
> + input = smp_load_acquire(&applespi->touchpad_input_dev);
> + if (!input)
> + return; /* touchpad isn't initialized yet */
> +
> + n = 0;
> +
> + for (i = 0; i < t->number_of_fingers; i++) {
for (n = 0, i = 0; i < ...; i++, n++) {
?
> + f = &t->fingers[i];
> + if (raw2int(f->touch_major) == 0)
> + continue;
> + applespi->pos[n].x = raw2int(f->abs_x);
> + applespi->pos[n].y = tp_info->y_min + tp_info->y_max -
> + raw2int(f->abs_y);
> + n++;
> +
> + if (debug & DBG_TP_DIM) {
> + #define UPDATE_DIMENSIONS(val, op, last) \
> + do { \
> + if (raw2int(val) op last) { \
> + last = raw2int(val); \
> + dim_updated = true; \
> + } \
> + } while (0)
> +
> + UPDATE_DIMENSIONS(f->abs_x, <, min_x);
> + UPDATE_DIMENSIONS(f->abs_x, >, max_x);
> + UPDATE_DIMENSIONS(f->abs_y, <, min_y);
> + UPDATE_DIMENSIONS(f->abs_y, >, max_y);
#undef ...
> + }
...and better to move this to separate helper function.
> + }
> +
> + if (debug & DBG_TP_DIM) {
> + if (dim_updated &&
> + ktime_ms_delta(ktime_get(), last_print) > 1000) {
> + printk(KERN_DEBUG
> + pr_fmt("New touchpad dimensions: %d %d %d %d\n"),
> + min_x, max_x, min_y, max_y);
> + dim_updated = false;
> + last_print = ktime_get();
> + }
> + }
Same, helper function.
> +
> + input_mt_assign_slots(input, applespi->slots, applespi->pos, n, 0);
> +
> + for (i = 0; i < n; i++)
> + report_finger_data(input, applespi->slots[i],
> + &applespi->pos[i], &t->fingers[i]);
> +
> + input_mt_sync_frame(input);
> + input_report_key(input, BTN_LEFT, t->clicked);
> +
> + input_sync(input);
> +}
> +
> +static unsigned int applespi_code_to_key(u8 code, int fn_pressed)
> +{
> + unsigned int key = applespi_scancodes[code];
> + const struct applespi_key_translation *trans;
> +
> + if (fnmode) {
> + int do_translate;
> +
> + trans = applespi_find_translation(applespi_fn_codes, key);
> + if (trans) {
> + if (trans->flags & APPLE_FLAG_FKEY)
> + do_translate = (fnmode == 2 && fn_pressed) ||
> + (fnmode == 1 && !fn_pressed);
> + else
> + do_translate = fn_pressed;
> +
> + if (do_translate)
> + key = trans->to;
> + }
> + }
> +
> + if (iso_layout) {
> + trans = applespi_find_translation(apple_iso_keyboard, key);
> + if (trans)
> + key = trans->to;
> + }
I would split this to three helpers like
static unsigned int ..._code_to_fn_key()
{
}
static unsigned int ..._code_to_iso_key()
{
}
static unsigned int ..._code_to_key()
{
if (fnmode)
key = ..._code_to_fn_key();
if (iso_layout)
key = ..._code_to_iso_key();
return key;
}
> +
> + return key;
> +}
> +static void applespi_remap_fn_key(struct keyboard_protocol
> + *keyboard_protocol)
Better to split like
static void
fn(struct ...);
> +{
> + unsigned char tmp;
> + unsigned long *modifiers = (unsigned long *)
> + &keyboard_protocol->modifiers;
Don't split casting from the rest, better
... var =
(type)...;
> +
> + if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> + !applespi_controlcodes[fnremap - 1])
> + return;
> +
> + tmp = keyboard_protocol->fn_pressed;
> + keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
> + if (tmp)
> + __set_bit(fnremap - 1, modifiers);
> + else
> + __clear_bit(fnremap - 1, modifiers);
> +}
> +
> +static void applespi_handle_keyboard_event(struct applespi_data *applespi,
> + struct keyboard_protocol
> + *keyboard_protocol)
Put this to one line, consider reformat like I mentioned above.
> +{
> + if (!still_pressed) {
> + key = applespi_code_to_key(
> + applespi->last_keys_pressed[i],
> + applespi->last_keys_fn_pressed[i]);
> + input_report_key(applespi->keyboard_input_dev, key, 0);
> + applespi->last_keys_fn_pressed[i] = 0;
> + }
This could come as
if (...)
continue;
...
> + }
> + memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
> + sizeof(applespi->last_keys_pressed));
applespi->last_keys_pressed = *keyboard_protocol->keys_pressed;
(if they are of the same type) ?
> +}
> +static void applespi_register_touchpad_device(struct applespi_data *applespi,
> + struct touchpad_info_protocol *rcvd_tp_info)
> +{
> + /* create touchpad input device */
> + touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
> +
Redundant.
> + if (!touchpad_input_dev) {
> + pr_err("Failed to allocate touchpad input device\n");
dev_err();
> + return;
Shouldn't we return an error to a caller?
> + }
> + /* register input device */
> + res = input_register_device(touchpad_input_dev);
> + if (res)
> + pr_err("Unabled to register touchpad input device (%d)\n", res);
> + else
if (ret) {
dev_err(...);
return ret;
}
Btw, ret, res, sts, result, ... Choose one.
> + /* touchpad_input_dev is read async in spi callback */
> + smp_store_release(&applespi->touchpad_input_dev,
> + touchpad_input_dev);
> +}
> +static bool applespi_verify_crc(struct applespi_data *applespi, u8 *buffer,
> + size_t buflen)
> +{
> + u16 crc;
> +
> + crc = crc16(0, buffer, buflen);
> + if (crc != 0) {
if (crc) {
> + dev_warn_ratelimited(&applespi->spi->dev,
> + "Received corrupted packet (crc mismatch)\n");
> + debug_print_header(DBG_RD_CRC);
> + debug_print_buffer(DBG_RD_CRC, "read ", buffer, buflen);
> +
> + return false;
> + }
> +
> + return true;
> +}
> +static void applespi_got_data(struct applespi_data *applespi)
> +{
> + } else if (packet->flags == PACKET_TYPE_READ &&
> + packet->device == PACKET_DEV_TPAD) {
> + struct touchpad_protocol *tp = &message->touchpad;
> +
> + size_t tp_len = sizeof(*tp) +
> + tp->number_of_fingers * sizeof(tp->fingers[0]);
Would be better
struct ...;
size_t ...;
... = ...;
if (...) {
> + if (le16_to_cpu(message->length) + 2 != tp_len) {
> + dev_warn_ratelimited(&applespi->spi->dev,
> + "Received corrupted packet (invalid message length)\n");
> + goto cleanup;
> + }
> + } else if (packet->flags == PACKET_TYPE_WRITE) {
> + applespi_handle_cmd_response(applespi, packet, message);
> + }
> +
> +cleanup:
err_msg_complete: ?
> + /* clean up */
Noise.
> + applespi->saved_msg_len = 0;
> +
> + applespi_msg_complete(applespi, packet->flags == PACKET_TYPE_WRITE,
> + true);
> +}
> +static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context)
> +{
> + struct applespi_data *applespi = context;
> + int sts;
> + unsigned long flags;
> +
> + debug_print_header(DBG_RD_IRQ);
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (!applespi->suspended) {
> + sts = applespi_async(applespi, &applespi->rd_m,
> + applespi_async_read_complete);
> + if (sts != 0)
if (sts)
> + pr_warn("Error queueing async read to device: %d\n",
> + sts);
> + else
> + applespi->read_active = true;
> + }
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> + return ACPI_INTERRUPT_HANDLED;
> +}
> +
> +static int applespi_get_saved_bl_level(void)
> +{
> + struct efivar_entry *efivar_entry;
> + u16 efi_data = 0;
> + unsigned long efi_data_len;
> + int sts;
> +
> + efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
> + if (!efivar_entry)
> + return -1;
-ENOMEM
> +
> + memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
> + sizeof(EFI_BL_LEVEL_NAME));
> + efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
> + efi_data_len = sizeof(efi_data);
> +
> + sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
> + if (sts && sts != -ENOENT)
> + pr_warn("Error getting backlight level from EFI vars: %d\n",
> + sts);
> +
> + kfree(efivar_entry);
> +
> + return efi_data;
> +}
> +static int applespi_probe(struct spi_device *spi)
> +{
> + applespi->msg_buf = devm_kmalloc(&spi->dev, MAX_PKTS_PER_MSG *
> + APPLESPI_PACKET_SIZE,
> + GFP_KERNEL);
devm_kmalloc_array();
> +
> + if (!applespi->tx_buffer || !applespi->tx_status ||
> + !applespi->rx_buffer || !applespi->msg_buf)
> + return -ENOMEM;
> + /*
> + * By default this device is not enable for wakeup; but USB keyboards
> + * generally are, so the expectation is that by default the keyboard
> + * will wake the system.
> + */
> + device_wakeup_enable(&spi->dev);
> + result = devm_led_classdev_register(&spi->dev,
> + &applespi->backlight_info);
> + if (result) {
> + pr_err("Unable to register keyboard backlight class dev (%d)\n",
> + result);
> + /* not fatal */
Noise. Instead use dev_warn().
> + }
> + /* done */
> + pr_info("spi-device probe done: %s\n", dev_name(&spi->dev));
Noise.
One may use "initcall_debug".
> + return 0;
> +}
> +
> +static int applespi_remove(struct spi_device *spi)
> +{
> + struct applespi_data *applespi = spi_get_drvdata(spi);
> + unsigned long flags;
> + /* shut things down */
Noise.
> + acpi_disable_gpe(NULL, applespi->gpe);
> + acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
> +
> + /* done */
> + pr_info("spi-device remove done: %s\n", dev_name(&spi->dev));
Noise.
It seems you still have wakeup enabled for the device.
> + return 0;
> +}
> +static int applespi_suspend(struct device *dev)
> +{
> + int rc;
Is it 6th type of naming for error code holding variable?
> + /* wait for all outstanding writes to finish */
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + applespi->drain = true;
> + wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
> + applespi->cmd_msg_lock);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
Helper? It's used in ->remove() AFAICS.
> + pr_info("spi-device suspend done.\n");
Noise.
One may use ftrace instead.
> + return 0;
> +}
> +
> +static int applespi_resume(struct device *dev)
> +{
> + pr_info("spi-device resume done.\n");
Ditto.
> +
> + return 0;
> +}
> +static const struct acpi_device_id applespi_acpi_match[] = {
> + { "APP000D", 0 },
> + { },
No comma, please.
> +};
> +MODULE_DEVICE_TABLE(acpi, applespi_acpi_match);
> +static struct spi_driver applespi_driver = {
> + .driver = {
> + .name = "applespi",
> + .owner = THIS_MODULE,
This set by macro.
> +
Redundant.
> + .acpi_match_table = ACPI_PTR(applespi_acpi_match),
Do you need ACPI_PTR() ?
> + .pm = &applespi_pm_ops,
> + },
> + .probe = applespi_probe,
> + .remove = applespi_remove,
> + .shutdown = applespi_shutdown,
> +};
> +
> +module_spi_driver(applespi_driver)
> +MODULE_LICENSE("GPL");
License mismatch.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 5/5] input: misc: bma150: Register input device after setting private data
From: Dmitry Torokhov @ 2019-02-06 19:23 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190206185307.GD174258@dtor-ws>
On Wed, Feb 06, 2019 at 10:53:07AM -0800, Dmitry Torokhov wrote:
> On Sat, Feb 02, 2019 at 04:18:06PM +0100, Paweł Chmiel wrote:
> > From: Jonathan Bakker <xc-racer2@live.ca>
> >
> > Otherwise we introduce a race condition where userspace can request input
> > before we're ready leading to null pointer dereference such as
> >
> > input: bma150 as /devices/platform/i2c-gpio-2/i2c-5/5-0038/input/input3
> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
> > pgd = (ptrval)
> > [00000018] *pgd=55dac831, *pte=00000000, *ppte=00000000
> > Internal error: Oops: 17 [#1] PREEMPT ARM
> > Modules linked in: bma150 input_polldev [last unloaded: bma150]
> > CPU: 0 PID: 2870 Comm: accelerometer Not tainted 5.0.0-rc3-dirty #46
> > Hardware name: Samsung S5PC110/S5PV210-based board
> > PC is at input_event+0x8/0x60
> > LR is at bma150_report_xyz+0x9c/0xe0 [bma150]
> > pc : [<80450f70>] lr : [<7f0a614c>] psr: 800d0013
> > sp : a4c1fd78 ip : 00000081 fp : 00020000
> > r10: 00000000 r9 : a5e2944c r8 : a7455000
> > r7 : 00000016 r6 : 00000101 r5 : a7617940 r4 : 80909048
> > r3 : fffffff2 r2 : 00000000 r1 : 00000003 r0 : 00000000
> > Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> > Control: 10c5387d Table: 54e34019 DAC: 00000051
> > Process accelerometer (pid: 2870, stack limit = 0x(ptrval))
> > Stackck: (0xa4c1fd78 to 0xa4c20000)
> > fd60: fffffff3 fc813f6c
> > fd80: 40410581 d7530ce3 a5e2817c a7617f00 a5e29404 a5e2817c 00000000 7f008324
> > fda0: a5e28000 8044f59c a5fdd9d0 a5e2945c a46a4a00 a5e29668 a7455000 80454f10
> > fdc0: 80909048 a5e29668 a5fdd9d0 a46a4a00 806316d0 00000000 a46a4a00 801df5f0
> > fde0: 00000000 d7530ce3 a4c1fec0 a46a4a00 00000000 a5fdd9d0 a46a4a08 801df53c
> > fe00: 00000000 801d74bc a4c1fec0 00000000 a4c1ff70 00000000 a7038da8 00000000
> > fe20: a46a4a00 801e91fc a411bbe0 801f2e88 00000004 00000000 80909048 00000041
> > fe40: 00000000 00020000 00000000 dead4ead a6a88da0 00000000 ffffe000 806fcae8
> > fe60: a4c1fec8 00000000 80909048 00000002 a5fdd9d0 a7660110 a411bab0 00000001
> > fe80: dead4ead ffffffff ffffffff a4c1fe8c a4c1fe8c d7530ce3 20000013 80909048
> > fea0: 80909048 a4c1ff70 00000001 fffff000 a4c1e000 00000005 00026038 801eabd8
> > fec0: a7660110 a411bab0 b9394901 00000006 a696201b 76fb3000 00000000 a7039720
> > fee0: a5fdd9d0 00000101 00000002 00000096 00000000 00000000 00000000 a4c1ff00
> > ff00: a6b310f4 805cb174 a6b310f4 00000010 00000fe0 00000010 a4c1e000 d7530ce3
> > ff20: 00000003 a5f41400 a5f41424 00000000 a6962000 00000000 00000003 00000002
> > ff40: ffffff9c 000a0000 80909048 d7530ce3 a6962000 00000003 80909048 ffffff9c
> > ff60: a6962000 801d890c 00000000 00000000 00020000 a7590000 00000004 00000100
> > ff80: 00000001 d7530ce3 000288b8 00026320 000288b8 00000005 80101204 a4c1e000
> > ffa0: 00000005 80101000 000288b8 00026320 000288b8 000a0000 00000000 00000000
> > ffc0: 000288b8 00026320 000288b8 00000005 7eef3bac 000264e8 00028ad8 00026038
> > ffe0: 00000005 7eef3300 76f76e91 76f78546 800d0030 000288b8 00000000 00000000
> > [<80450f70>] (input_event) from [<a5e2817c>] (0xa5e2817c)
> > Code: e1a08148 eaffffa8 e351001f 812fff1e (e590c018)
> > ---[ end trace 1c691ee85f2ff243 ]---
> >
> > Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>
> Applied, thank you.
Actually I'll move it to the current release and mark for sable.
>
> > ---
> > drivers/input/misc/bma150.c | 15 +++------------
> > 1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> > index 1cdc8ce97968..64caf43e5bca 100644
> > --- a/drivers/input/misc/bma150.c
> > +++ b/drivers/input/misc/bma150.c
> > @@ -470,7 +470,6 @@ static void bma150_init_input_device(struct bma150_data *bma150,
> > static int bma150_register_input_device(struct bma150_data *bma150)
> > {
> > struct input_dev *idev;
> > - int error;
> >
> > idev = devm_input_allocate_device(&bma150->client->dev);
> > if (!idev)
> > @@ -482,18 +481,14 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> > idev->close = bma150_irq_close;
> > input_set_drvdata(idev, bma150);
> >
> > - error = input_register_device(idev);
> > - if (error)
> > - return error;
> > -
> > bma150->input = idev;
> > - return 0;
> > +
> > + return input_register_device(idev);
> > }
> >
> > static int bma150_register_polled_device(struct bma150_data *bma150)
> > {
> > struct input_polled_dev *ipoll_dev;
> > - int error;
> >
> > ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> > if (!ipoll_dev)
> > @@ -509,14 +504,10 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> >
> > bma150_init_input_device(bma150, ipoll_dev->input);
> >
> > - error = input_register_polled_device(ipoll_dev);
> > - if (error)
> > - return error;
> > -
> > bma150->input_polled = ipoll_dev;
> > bma150->input = ipoll_dev->input;
> >
> > - return 0;
> > + return input_register_polled_device(ipoll_dev);
> > }
> >
> > int bma150_cfg_from_of(struct device_node *np)
> > --
> > 2.17.1
> >
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: cap11xx - switch to using set_brightness_blocking()
From: Jacek Anaszewski @ 2019-02-06 19:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: Sven Van Asbroeck, linux-kernel
In-Reply-To: <20190205222050.GA145676@dtor-ws>
Hi Dmitry,
On 2/5/19 11:20 PM, Dmitry Torokhov wrote:
> Updating LED state requires access to regmap and therefore we may sleep, so
> we could not do that directly form set_brightness() method. Historically
> we used private work to adjust the brightness, but with the introduction of
> set_brightness_blocking() we no longer need it.
>
> As a bonus, not having our own work item means we do not have
> use-after-free issue as we neglected to cancel outstanding work on driver
> unbind.
>
> Reported-by: Sven Van Asbroeck <thesven73@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/cap11xx.c | 47 ++++++++++++++------------------
> 1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 312916f99597..c0baf323ddda 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -75,9 +75,8 @@
> struct cap11xx_led {
> struct cap11xx_priv *priv;
> struct led_classdev cdev;
> - struct work_struct work;
> u32 reg;
> - enum led_brightness new_brightness;
> + enum led_brightness brightness;
> };
> #endif
>
> @@ -233,30 +232,28 @@ static void cap11xx_input_close(struct input_dev *idev)
> }
>
> #ifdef CONFIG_LEDS_CLASS
> -static void cap11xx_led_work(struct work_struct *work)
> -{
> - struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
> - struct cap11xx_priv *priv = led->priv;
> - int value = led->new_brightness;
> -
> - /*
> - * All LEDs share the same duty cycle as this is a HW limitation.
> - * Brightness levels per LED are either 0 (OFF) and 1 (ON).
> - */
> - regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
> - BIT(led->reg), value ? BIT(led->reg) : 0);
> -}
> -
> -static void cap11xx_led_set(struct led_classdev *cdev,
> - enum led_brightness value)
> +static int cap11xx_led_set(struct led_classdev *cdev,
> + enum led_brightness value)
> {
> struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> + struct cap11xx_priv *priv = led->priv;
> + int error = 0;
> +
> + if (led->brightness != value) {
I'd say this check is not needed. If registers are not marked volatile
then regmap should not do the actual write to the hardware if the value
is equal to the cached one.
> + /*
> + * All LEDs share the same duty cycle as this is a HW
> + * limitation. Brightness levels per LED are either
> + * 0 (OFF) and 1 (ON).
> + */
> + error = regmap_update_bits(priv->regmap,
> + CAP11XX_REG_LED_OUTPUT_CONTROL,
> + BIT(led->reg),
> + value ? BIT(led->reg) : 0);
> + if (!error)
> + led->brightness = value;
> + }
>
> - if (led->new_brightness == value)
> - return;
> -
> - led->new_brightness = value;
> - schedule_work(&led->work);
> + return error;
> }
>
> static int cap11xx_init_leds(struct device *dev,
> @@ -299,7 +296,7 @@ static int cap11xx_init_leds(struct device *dev,
> led->cdev.default_trigger =
> of_get_property(child, "linux,default-trigger", NULL);
> led->cdev.flags = 0;
> - led->cdev.brightness_set = cap11xx_led_set;
> + led->cdev.brightness_set_blocking = cap11xx_led_set;
> led->cdev.max_brightness = 1;
> led->cdev.brightness = LED_OFF;
>
> @@ -312,8 +309,6 @@ static int cap11xx_init_leds(struct device *dev,
> led->reg = reg;
> led->priv = priv;
>
> - INIT_WORK(&led->work, cap11xx_led_work);
> -
> error = devm_led_classdev_register(dev, &led->cdev);
> if (error) {
> of_node_put(child);
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v2 5/5] input: misc: bma150: Register input device after setting private data
From: Dmitry Torokhov @ 2019-02-06 18:53 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190202151806.9064-6-pawel.mikolaj.chmiel@gmail.com>
On Sat, Feb 02, 2019 at 04:18:06PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> Otherwise we introduce a race condition where userspace can request input
> before we're ready leading to null pointer dereference such as
>
> input: bma150 as /devices/platform/i2c-gpio-2/i2c-5/5-0038/input/input3
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = (ptrval)
> [00000018] *pgd=55dac831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] PREEMPT ARM
> Modules linked in: bma150 input_polldev [last unloaded: bma150]
> CPU: 0 PID: 2870 Comm: accelerometer Not tainted 5.0.0-rc3-dirty #46
> Hardware name: Samsung S5PC110/S5PV210-based board
> PC is at input_event+0x8/0x60
> LR is at bma150_report_xyz+0x9c/0xe0 [bma150]
> pc : [<80450f70>] lr : [<7f0a614c>] psr: 800d0013
> sp : a4c1fd78 ip : 00000081 fp : 00020000
> r10: 00000000 r9 : a5e2944c r8 : a7455000
> r7 : 00000016 r6 : 00000101 r5 : a7617940 r4 : 80909048
> r3 : fffffff2 r2 : 00000000 r1 : 00000003 r0 : 00000000
> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: 54e34019 DAC: 00000051
> Process accelerometer (pid: 2870, stack limit = 0x(ptrval))
> Stackck: (0xa4c1fd78 to 0xa4c20000)
> fd60: fffffff3 fc813f6c
> fd80: 40410581 d7530ce3 a5e2817c a7617f00 a5e29404 a5e2817c 00000000 7f008324
> fda0: a5e28000 8044f59c a5fdd9d0 a5e2945c a46a4a00 a5e29668 a7455000 80454f10
> fdc0: 80909048 a5e29668 a5fdd9d0 a46a4a00 806316d0 00000000 a46a4a00 801df5f0
> fde0: 00000000 d7530ce3 a4c1fec0 a46a4a00 00000000 a5fdd9d0 a46a4a08 801df53c
> fe00: 00000000 801d74bc a4c1fec0 00000000 a4c1ff70 00000000 a7038da8 00000000
> fe20: a46a4a00 801e91fc a411bbe0 801f2e88 00000004 00000000 80909048 00000041
> fe40: 00000000 00020000 00000000 dead4ead a6a88da0 00000000 ffffe000 806fcae8
> fe60: a4c1fec8 00000000 80909048 00000002 a5fdd9d0 a7660110 a411bab0 00000001
> fe80: dead4ead ffffffff ffffffff a4c1fe8c a4c1fe8c d7530ce3 20000013 80909048
> fea0: 80909048 a4c1ff70 00000001 fffff000 a4c1e000 00000005 00026038 801eabd8
> fec0: a7660110 a411bab0 b9394901 00000006 a696201b 76fb3000 00000000 a7039720
> fee0: a5fdd9d0 00000101 00000002 00000096 00000000 00000000 00000000 a4c1ff00
> ff00: a6b310f4 805cb174 a6b310f4 00000010 00000fe0 00000010 a4c1e000 d7530ce3
> ff20: 00000003 a5f41400 a5f41424 00000000 a6962000 00000000 00000003 00000002
> ff40: ffffff9c 000a0000 80909048 d7530ce3 a6962000 00000003 80909048 ffffff9c
> ff60: a6962000 801d890c 00000000 00000000 00020000 a7590000 00000004 00000100
> ff80: 00000001 d7530ce3 000288b8 00026320 000288b8 00000005 80101204 a4c1e000
> ffa0: 00000005 80101000 000288b8 00026320 000288b8 000a0000 00000000 00000000
> ffc0: 000288b8 00026320 000288b8 00000005 7eef3bac 000264e8 00028ad8 00026038
> ffe0: 00000005 7eef3300 76f76e91 76f78546 800d0030 000288b8 00000000 00000000
> [<80450f70>] (input_event) from [<a5e2817c>] (0xa5e2817c)
> Code: e1a08148 eaffffa8 e351001f 812fff1e (e590c018)
> ---[ end trace 1c691ee85f2ff243 ]---
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/bma150.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> index 1cdc8ce97968..64caf43e5bca 100644
> --- a/drivers/input/misc/bma150.c
> +++ b/drivers/input/misc/bma150.c
> @@ -470,7 +470,6 @@ static void bma150_init_input_device(struct bma150_data *bma150,
> static int bma150_register_input_device(struct bma150_data *bma150)
> {
> struct input_dev *idev;
> - int error;
>
> idev = devm_input_allocate_device(&bma150->client->dev);
> if (!idev)
> @@ -482,18 +481,14 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> idev->close = bma150_irq_close;
> input_set_drvdata(idev, bma150);
>
> - error = input_register_device(idev);
> - if (error)
> - return error;
> -
> bma150->input = idev;
> - return 0;
> +
> + return input_register_device(idev);
> }
>
> static int bma150_register_polled_device(struct bma150_data *bma150)
> {
> struct input_polled_dev *ipoll_dev;
> - int error;
>
> ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> if (!ipoll_dev)
> @@ -509,14 +504,10 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
>
> bma150_init_input_device(bma150, ipoll_dev->input);
>
> - error = input_register_polled_device(ipoll_dev);
> - if (error)
> - return error;
> -
> bma150->input_polled = ipoll_dev;
> bma150->input = ipoll_dev->input;
>
> - return 0;
> + return input_register_polled_device(ipoll_dev);
> }
>
> int bma150_cfg_from_of(struct device_node *np)
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/5] input: misc: bma150: Use managed resources helpers
From: Dmitry Torokhov @ 2019-02-06 18:52 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190202151806.9064-3-pawel.mikolaj.chmiel@gmail.com>
On Sat, Feb 02, 2019 at 04:18:03PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> The driver can be cleaned up by using managed resource helpers
>
> Changes from v1:
> - Correct devm input unregistering
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/bma150.c | 44 ++++++++++---------------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> index 1efcfdf9f8a8..79acaaf86b7e 100644
> --- a/drivers/input/misc/bma150.c
> +++ b/drivers/input/misc/bma150.c
> @@ -471,7 +471,7 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> struct input_dev *idev;
> int error;
>
> - idev = input_allocate_device();
> + idev = devm_input_allocate_device(&bma150->client->dev);
> if (!idev)
> return -ENOMEM;
>
> @@ -482,10 +482,8 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> input_set_drvdata(idev, bma150);
>
> error = input_register_device(idev);
> - if (error) {
> - input_free_device(idev);
> + if (error)
> return error;
> - }
>
> bma150->input = idev;
> return 0;
> @@ -496,7 +494,7 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> struct input_polled_dev *ipoll_dev;
> int error;
>
> - ipoll_dev = input_allocate_polled_device();
> + ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> if (!ipoll_dev)
> return -ENOMEM;
>
> @@ -511,10 +509,8 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> bma150_init_input_device(bma150, ipoll_dev->input);
>
> error = input_register_polled_device(ipoll_dev);
> - if (error) {
> - input_free_polled_device(ipoll_dev);
> + if (error)
> return error;
> - }
>
> bma150->input_polled = ipoll_dev;
> bma150->input = ipoll_dev->input;
> @@ -543,7 +539,8 @@ static int bma150_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - bma150 = kzalloc(sizeof(struct bma150_data), GFP_KERNEL);
> + bma150 = devm_kzalloc(&client->dev, sizeof(struct bma150_data),
> + GFP_KERNEL);
> if (!bma150)
> return -ENOMEM;
>
> @@ -556,7 +553,7 @@ static int bma150_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "IRQ GPIO conf. error %d, error %d\n",
> client->irq, error);
> - goto err_free_mem;
> + return error;
> }
> }
> cfg = &pdata->cfg;
> @@ -566,14 +563,14 @@ static int bma150_probe(struct i2c_client *client,
>
> error = bma150_initialize(bma150, cfg);
> if (error)
> - goto err_free_mem;
> + return error;
>
> if (client->irq > 0) {
> error = bma150_register_input_device(bma150);
> if (error)
> - goto err_free_mem;
> + return error;
>
> - error = request_threaded_irq(client->irq,
> + error = devm_request_threaded_irq(&client->dev, client->irq,
> NULL, bma150_irq_thread,
> IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> BMA150_DRIVER, bma150);
> @@ -581,13 +578,12 @@ static int bma150_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "irq request failed %d, error %d\n",
> client->irq, error);
> - input_unregister_device(bma150->input);
> - goto err_free_mem;
> + return error;
> }
> } else {
> error = bma150_register_polled_device(bma150);
> if (error)
> - goto err_free_mem;
> + return error;
> }
>
> i2c_set_clientdata(client, bma150);
> @@ -595,28 +591,12 @@ static int bma150_probe(struct i2c_client *client,
> pm_runtime_enable(&client->dev);
>
> return 0;
> -
> -err_free_mem:
> - kfree(bma150);
> - return error;
> }
>
> static int bma150_remove(struct i2c_client *client)
> {
> - struct bma150_data *bma150 = i2c_get_clientdata(client);
> -
> pm_runtime_disable(&client->dev);
>
> - if (client->irq > 0) {
> - free_irq(client->irq, bma150);
> - input_unregister_device(bma150->input);
> - } else {
> - input_unregister_polled_device(bma150->input_polled);
> - input_free_polled_device(bma150->input_polled);
> - }
> -
> - kfree(bma150);
> -
> return 0;
> }
>
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Brian Masney @ 2019-02-06 18:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206181822.GB174258@dtor-ws>
On Wed, Feb 06, 2019 at 10:18:22AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 05, 2019 at 08:33:29PM -0500, Brian Masney wrote:
> > This patch adds device device tree bindings for the vibrator found on
> > the LG Nexus 5 (hammerhead) phone.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
>
> This should go through Qualcomm tree with the other bindings?
Yes, this will go through Andy's tree.
Brian
^ permalink raw reply
* Re: [PATCH v4 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Dmitry Torokhov @ 2019-02-06 18:18 UTC (permalink / raw)
To: Brian Masney
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-4-masneyb@onstation.org>
On Tue, Feb 05, 2019 at 08:33:29PM -0500, Brian Masney wrote:
> This patch adds device device tree bindings for the vibrator found on
> the LG Nexus 5 (hammerhead) phone.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
This should go through Qualcomm tree with the other bindings?
> ---
> Changes since v3:
> - None
>
> .../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> index b3b04736a159..1fd9f429f34a 100644
> --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -5,6 +5,7 @@
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/clock/qcom,mmcc-msm8974.h>
>
> / {
> model = "LGE MSM 8974 HAMMERHEAD";
> @@ -306,6 +307,36 @@
> input-enable;
> };
> };
> +
> + vibrator_pin: vibrator {
> + pwm {
> + pins = "gpio27";
> + function = "gp1_clk";
> +
> + drive-strength = <6>;
> + bias-disable;
> + };
> +
> + enable {
> + pins = "gpio60";
> + function = "gpio";
> + };
> + };
> + };
> +
> + vibrator@fd8c3450 {
> + compatible = "qcom,msm8974-vibrator";
> + reg = <0xfd8c3450 0x400>;
> +
> + vcc-supply = <&pm8941_l19>;
> +
> + clocks = <&mmcc CAMSS_GP1_CLK>;
> + clock-names = "pwm";
> +
> + enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&vibrator_pin>;
> };
>
> sdhci@f9824900 {
> --
> 2.17.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 2/3] Input: add new vibrator driver for various MSM SOCs
From: Dmitry Torokhov @ 2019-02-06 18:16 UTC (permalink / raw)
To: Brian Masney
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-3-masneyb@onstation.org>
On Tue, Feb 05, 2019 at 08:33:28PM -0500, Brian Masney wrote:
> This patch adds a new vibrator driver that supports various Qualcomm
> MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied together with binding patch, thank you.
> ---
> Changes since v3:
> - Made msm_vibrator_write a function instead of a macro.
> - Check for -EPROBE_DEFER for gpio and clk for consistency with the
> regulator.
> - Check for NULL return value from devm_ioremap() instead of using
> IS_ERR()
> - Don't set dev.parent
> - Use platform_get_drvdata() in suspend and resume
>
> drivers/input/misc/Kconfig | 10 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/msm-vibrator.c | 282 ++++++++++++++++++++++++++++++
> 3 files changed, 293 insertions(+)
> create mode 100644 drivers/input/misc/msm-vibrator.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2be9bc5..e39aef84f357 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -117,6 +117,16 @@ config INPUT_E3X0_BUTTON
> To compile this driver as a module, choose M here: the
> module will be called e3x0_button.
>
> +config INPUT_MSM_VIBRATOR
> + tristate "Qualcomm MSM vibrator driver"
> + select INPUT_FF_MEMLESS
> + help
> + Support for the vibrator that is found on various Qualcomm MSM
> + SOCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called msm_vibrator.
> +
> config INPUT_PCSPKR
> tristate "PC Speaker support"
> depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1ff68f..96a6419cb1f2 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
> obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o
> obj-$(CONFIG_INPUT_MC13783_PWRBUTTON) += mc13783-pwrbutton.o
> obj-$(CONFIG_INPUT_MMA8450) += mma8450.o
> +obj-$(CONFIG_INPUT_MSM_VIBRATOR) += msm-vibrator.o
> obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
> obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
> obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
> diff --git a/drivers/input/misc/msm-vibrator.c b/drivers/input/misc/msm-vibrator.c
> new file mode 100644
> index 000000000000..c06941021447
> --- /dev/null
> +++ b/drivers/input/misc/msm-vibrator.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Qualcomm MSM vibrator driver
> + *
> + * Copyright (c) 2018 Brian Masney <masneyb@onstation.org>
> + *
> + * Based on qcom,pwm-vibrator.c from:
> + * Copyright (c) 2018 Jonathan Marek <jonathan@marek.ca>
> + *
> + * Based on msm_pwm_vibrator.c from downstream Android sources:
> + * Copyright (C) 2009-2014 LGE, Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define REG_CMD_RCGR 0x00
> +#define REG_CFG_RCGR 0x04
> +#define REG_M 0x08
> +#define REG_N 0x0C
> +#define REG_D 0x10
> +#define REG_CBCR 0x24
> +#define MMSS_CC_M_DEFAULT 1
> +
> +struct msm_vibrator {
> + struct input_dev *input;
> + struct mutex mutex;
> + struct work_struct worker;
> + void __iomem *base;
> + struct regulator *vcc;
> + struct clk *clk;
> + struct gpio_desc *enable_gpio;
> + u16 magnitude;
> + bool enabled;
> +};
> +
> +static void msm_vibrator_write(struct msm_vibrator *vibrator, int offset,
> + u32 value)
> +{
> + writel(value, vibrator->base + offset);
> +}
> +
> +static int msm_vibrator_start(struct msm_vibrator *vibrator)
> +{
> + int d_reg_val, ret = 0;
> +
> + mutex_lock(&vibrator->mutex);
> +
> + if (!vibrator->enabled) {
> + ret = clk_set_rate(vibrator->clk, 24000);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to set clock rate: %d\n", ret);
> + goto unlock;
> + }
> +
> + ret = clk_prepare_enable(vibrator->clk);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to enable clock: %d\n", ret);
> + goto unlock;
> + }
> +
> + ret = regulator_enable(vibrator->vcc);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to enable regulator: %d\n", ret);
> + clk_disable(vibrator->clk);
> + goto unlock;
> + }
> +
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
> +
> + vibrator->enabled = true;
> + }
> +
> + d_reg_val = 127 - ((126 * vibrator->magnitude) / 0xffff);
> + msm_vibrator_write(vibrator, REG_CFG_RCGR,
> + (2 << 12) | /* dual edge mode */
> + (0 << 8) | /* cxo */
> + (7 << 0));
> + msm_vibrator_write(vibrator, REG_M, 1);
> + msm_vibrator_write(vibrator, REG_N, 128);
> + msm_vibrator_write(vibrator, REG_D, d_reg_val);
> + msm_vibrator_write(vibrator, REG_CMD_RCGR, 1);
> + msm_vibrator_write(vibrator, REG_CBCR, 1);
> +
> +unlock:
> + mutex_unlock(&vibrator->mutex);
> +
> + return ret;
> +}
> +
> +static void msm_vibrator_stop(struct msm_vibrator *vibrator)
> +{
> + mutex_lock(&vibrator->mutex);
> +
> + if (vibrator->enabled) {
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
> + regulator_disable(vibrator->vcc);
> + clk_disable(vibrator->clk);
> + vibrator->enabled = false;
> + }
> +
> + mutex_unlock(&vibrator->mutex);
> +}
> +
> +static void msm_vibrator_worker(struct work_struct *work)
> +{
> + struct msm_vibrator *vibrator = container_of(work,
> + struct msm_vibrator,
> + worker);
> +
> + if (vibrator->magnitude)
> + msm_vibrator_start(vibrator);
> + else
> + msm_vibrator_stop(vibrator);
> +}
> +
> +static int msm_vibrator_play_effect(struct input_dev *dev, void *data,
> + struct ff_effect *effect)
> +{
> + struct msm_vibrator *vibrator = input_get_drvdata(dev);
> +
> + mutex_lock(&vibrator->mutex);
> +
> + if (effect->u.rumble.strong_magnitude > 0)
> + vibrator->magnitude = effect->u.rumble.strong_magnitude;
> + else
> + vibrator->magnitude = effect->u.rumble.weak_magnitude;
> +
> + mutex_unlock(&vibrator->mutex);
> +
> + schedule_work(&vibrator->worker);
> +
> + return 0;
> +}
> +
> +static void msm_vibrator_close(struct input_dev *input)
> +{
> + struct msm_vibrator *vibrator = input_get_drvdata(input);
> +
> + cancel_work_sync(&vibrator->worker);
> + msm_vibrator_stop(vibrator);
> +}
> +
> +static int msm_vibrator_probe(struct platform_device *pdev)
> +{
> + struct msm_vibrator *vibrator;
> + struct resource *res;
> + int ret;
> +
> + vibrator = devm_kzalloc(&pdev->dev, sizeof(*vibrator), GFP_KERNEL);
> + if (!vibrator)
> + return -ENOMEM;
> +
> + vibrator->input = devm_input_allocate_device(&pdev->dev);
> + if (!vibrator->input)
> + return -ENOMEM;
> +
> + vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
> + if (IS_ERR(vibrator->vcc)) {
> + if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
> + PTR_ERR(vibrator->vcc));
> + return PTR_ERR(vibrator->vcc);
> + }
> +
> + vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(vibrator->enable_gpio)) {
> + if (PTR_ERR(vibrator->enable_gpio) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
> + PTR_ERR(vibrator->enable_gpio));
> + return PTR_ERR(vibrator->enable_gpio);
> + }
> +
> + vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(vibrator->clk)) {
> + if (PTR_ERR(vibrator->clk) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
> + PTR_ERR(vibrator->clk));
> + return PTR_ERR(vibrator->clk);
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get platform resource\n");
> + return -ENODEV;
> + }
> +
> + vibrator->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!vibrator->base) {
> + dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
> + PTR_ERR(vibrator->base));
> + return -ENOMEM;
> + }
> +
> + vibrator->enabled = false;
> + mutex_init(&vibrator->mutex);
> + INIT_WORK(&vibrator->worker, msm_vibrator_worker);
> +
> + vibrator->input->name = "msm-vibrator";
> + vibrator->input->id.bustype = BUS_HOST;
> + vibrator->input->close = msm_vibrator_close;
> +
> + input_set_drvdata(vibrator->input, vibrator);
> + input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
> +
> + ret = input_ff_create_memless(vibrator->input, NULL,
> + msm_vibrator_play_effect);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
> + return ret;
> + }
> +
> + ret = input_register_device(vibrator->input);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register input device: %d", ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
> +
> + cancel_work_sync(&vibrator->worker);
> +
> + if (vibrator->enabled)
> + msm_vibrator_stop(vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
> +
> + if (vibrator->enabled)
> + msm_vibrator_start(vibrator);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(msm_vibrator_pm_ops, msm_vibrator_suspend,
> + msm_vibrator_resume);
> +
> +static const struct of_device_id msm_vibrator_of_match[] = {
> + { .compatible = "qcom,msm8226-vibrator" },
> + { .compatible = "qcom,msm8974-vibrator" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, msm_vibrator_of_match);
> +
> +static struct platform_driver msm_vibrator_driver = {
> + .probe = msm_vibrator_probe,
> + .driver = {
> + .name = "msm-vibrator",
> + .pm = &msm_vibrator_pm_ops,
> + .of_match_table = of_match_ptr(msm_vibrator_of_match),
> + },
> +};
> +module_platform_driver(msm_vibrator_driver);
> +
> +MODULE_AUTHOR("Brian Masney <masneyb@onstation.org>");
> +MODULE_DESCRIPTION("Qualcomm MSM vibrator driver");
> +MODULE_LICENSE("GPL");
> --
> 2.17.2
>
--
Dmitry
^ permalink raw reply
* [PATCH] Input: tm2-touchkey - acknowledge that setting brightness is a blocking call
From: Dmitry Torokhov @ 2019-02-06 18:16 UTC (permalink / raw)
To: linux-input; +Cc: Paweł Chmiel, Jonathan Bakker, Andi Shyti, linux-kernel
We need to access I2C bus when switching brightness, and that may block,
therefore we have to set stmfts_brightness_set() as LED's
brightness_set_blocking() method.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/tm2-touchkey.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/input/keyboard/tm2-touchkey.c b/drivers/input/keyboard/tm2-touchkey.c
index 7dbef96559d2..d4455f3a5cf1 100644
--- a/drivers/input/keyboard/tm2-touchkey.c
+++ b/drivers/input/keyboard/tm2-touchkey.c
@@ -78,7 +78,7 @@ static struct touchkey_variant aries_touchkey_variant = {
.cmd_led_off = ARIES_TOUCHKEY_CMD_LED_OFF,
};
-static void tm2_touchkey_led_brightness_set(struct led_classdev *led_dev,
+static int tm2_touchkey_led_brightness_set(struct led_classdev *led_dev,
enum led_brightness brightness)
{
struct tm2_touchkey_data *touchkey =
@@ -97,9 +97,8 @@ static void tm2_touchkey_led_brightness_set(struct led_classdev *led_dev,
if (!touchkey->variant->fixed_regulator)
regulator_set_voltage(touchkey->vdd, volt, volt);
- if (touchkey->variant->no_reg)
- i2c_smbus_write_byte(touchkey->client, data);
- else
+ return touchkey->variant->no_reg ?
+ i2c_smbus_write_byte(touchkey->client, data) :
i2c_smbus_write_byte_data(touchkey->client,
touchkey->variant->base_reg, data);
}
@@ -270,7 +269,8 @@ static int tm2_touchkey_probe(struct i2c_client *client,
touchkey->led_dev.name = TM2_TOUCHKEY_DEV_NAME;
touchkey->led_dev.brightness = LED_ON;
touchkey->led_dev.max_brightness = LED_ON;
- touchkey->led_dev.brightness_set = tm2_touchkey_led_brightness_set;
+ touchkey->led_dev.brightness_set_blocking =
+ tm2_touchkey_led_brightness_set;
error = devm_led_classdev_register(&client->dev, &touchkey->led_dev);
if (error) {
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: ims-pcu - switch to using brightness_set_blocking()
From: Dmitry Torokhov @ 2019-02-06 18:08 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, Chris Healy
Now that LEDs core allows "blocking" flavor of "set brightness" method we
can use it and get rid of private work item.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 3d51175c4d72..74cf3b612f05 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -39,8 +39,6 @@ struct ims_pcu_gamepad {
struct ims_pcu_backlight {
struct led_classdev cdev;
- struct work_struct work;
- enum led_brightness desired_brightness;
char name[32];
};
@@ -949,14 +947,14 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
#define IMS_PCU_MAX_BRIGHTNESS 31998
-static void ims_pcu_backlight_work(struct work_struct *work)
+static int ims_pcu_backlight_set_brightness(struct led_classdev *cdev,
+ enum led_brightness value)
{
struct ims_pcu_backlight *backlight =
- container_of(work, struct ims_pcu_backlight, work);
+ container_of(cdev, struct ims_pcu_backlight, cdev);
struct ims_pcu *pcu =
container_of(backlight, struct ims_pcu, backlight);
- int desired_brightness = backlight->desired_brightness;
- __le16 br_val = cpu_to_le16(desired_brightness);
+ __le16 br_val = cpu_to_le16(value);
int error;
mutex_lock(&pcu->cmd_mutex);
@@ -966,19 +964,11 @@ static void ims_pcu_backlight_work(struct work_struct *work)
if (error && error != -ENODEV)
dev_warn(pcu->dev,
"Failed to set desired brightness %u, error: %d\n",
- desired_brightness, error);
+ value, error);
mutex_unlock(&pcu->cmd_mutex);
-}
-static void ims_pcu_backlight_set_brightness(struct led_classdev *cdev,
- enum led_brightness value)
-{
- struct ims_pcu_backlight *backlight =
- container_of(cdev, struct ims_pcu_backlight, cdev);
-
- backlight->desired_brightness = value;
- schedule_work(&backlight->work);
+ return error;
}
static enum led_brightness
@@ -1015,14 +1005,14 @@ static int ims_pcu_setup_backlight(struct ims_pcu *pcu)
struct ims_pcu_backlight *backlight = &pcu->backlight;
int error;
- INIT_WORK(&backlight->work, ims_pcu_backlight_work);
snprintf(backlight->name, sizeof(backlight->name),
"pcu%d::kbd_backlight", pcu->device_no);
backlight->cdev.name = backlight->name;
backlight->cdev.max_brightness = IMS_PCU_MAX_BRIGHTNESS;
backlight->cdev.brightness_get = ims_pcu_backlight_get_brightness;
- backlight->cdev.brightness_set = ims_pcu_backlight_set_brightness;
+ backlight->cdev.brightness_set_blocking =
+ ims_pcu_backlight_set_brightness;
error = led_classdev_register(pcu->dev, &backlight->cdev);
if (error) {
@@ -1040,7 +1030,6 @@ static void ims_pcu_destroy_backlight(struct ims_pcu *pcu)
struct ims_pcu_backlight *backlight = &pcu->backlight;
led_classdev_unregister(&backlight->cdev);
- cancel_work_sync(&backlight->work);
}
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: cap11xx - switch to using set_brightness_blocking()
From: Sven Van Asbroeck @ 2019-02-06 14:33 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Jacek Anaszewski, Linux Kernel Mailing List
In-Reply-To: <20190205222050.GA145676@dtor-ws>
On Tue, Feb 5, 2019 at 5:20 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Updating LED state requires access to regmap and therefore we may sleep, so
> we could not do that directly form set_brightness() method. Historically
> we used private work to adjust the brightness, but with the introduction of
> set_brightness_blocking() we no longer need it.
>
Elegant solution, nice !
I read the patch to verify that the user-after-free is now gone, but
obviously I cannot test,
as I have no cap11xx hardware. For what it's worth:
Reviewed-by: Sven Van Asbroeck <TheSven73@googlemail.com>
^ permalink raw reply
* [PATCH v4 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Brian Masney @ 2019-02-06 1:33 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross
Cc: david.brown, robh+dt, mark.rutland, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-1-masneyb@onstation.org>
This patch adds device device tree bindings for the vibrator found on
the LG Nexus 5 (hammerhead) phone.
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Changes since v3:
- None
.../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index b3b04736a159..1fd9f429f34a 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -5,6 +5,7 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/input/input.h>
#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/clock/qcom,mmcc-msm8974.h>
/ {
model = "LGE MSM 8974 HAMMERHEAD";
@@ -306,6 +307,36 @@
input-enable;
};
};
+
+ vibrator_pin: vibrator {
+ pwm {
+ pins = "gpio27";
+ function = "gp1_clk";
+
+ drive-strength = <6>;
+ bias-disable;
+ };
+
+ enable {
+ pins = "gpio60";
+ function = "gpio";
+ };
+ };
+ };
+
+ vibrator@fd8c3450 {
+ compatible = "qcom,msm8974-vibrator";
+ reg = <0xfd8c3450 0x400>;
+
+ vcc-supply = <&pm8941_l19>;
+
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+ clock-names = "pwm";
+
+ enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&vibrator_pin>;
};
sdhci@f9824900 {
--
2.17.2
^ permalink raw reply related
* [PATCH v4 2/3] Input: add new vibrator driver for various MSM SOCs
From: Brian Masney @ 2019-02-06 1:33 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross
Cc: david.brown, robh+dt, mark.rutland, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-1-masneyb@onstation.org>
This patch adds a new vibrator driver that supports various Qualcomm
MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Changes since v3:
- Made msm_vibrator_write a function instead of a macro.
- Check for -EPROBE_DEFER for gpio and clk for consistency with the
regulator.
- Check for NULL return value from devm_ioremap() instead of using
IS_ERR()
- Don't set dev.parent
- Use platform_get_drvdata() in suspend and resume
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/msm-vibrator.c | 282 ++++++++++++++++++++++++++++++
3 files changed, 293 insertions(+)
create mode 100644 drivers/input/misc/msm-vibrator.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index ca59a2be9bc5..e39aef84f357 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -117,6 +117,16 @@ config INPUT_E3X0_BUTTON
To compile this driver as a module, choose M here: the
module will be called e3x0_button.
+config INPUT_MSM_VIBRATOR
+ tristate "Qualcomm MSM vibrator driver"
+ select INPUT_FF_MEMLESS
+ help
+ Support for the vibrator that is found on various Qualcomm MSM
+ SOCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called msm_vibrator.
+
config INPUT_PCSPKR
tristate "PC Speaker support"
depends on PCSPKR_PLATFORM
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 9d0f9d1ff68f..96a6419cb1f2 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o
obj-$(CONFIG_INPUT_MC13783_PWRBUTTON) += mc13783-pwrbutton.o
obj-$(CONFIG_INPUT_MMA8450) += mma8450.o
+obj-$(CONFIG_INPUT_MSM_VIBRATOR) += msm-vibrator.o
obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
diff --git a/drivers/input/misc/msm-vibrator.c b/drivers/input/misc/msm-vibrator.c
new file mode 100644
index 000000000000..c06941021447
--- /dev/null
+++ b/drivers/input/misc/msm-vibrator.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Qualcomm MSM vibrator driver
+ *
+ * Copyright (c) 2018 Brian Masney <masneyb@onstation.org>
+ *
+ * Based on qcom,pwm-vibrator.c from:
+ * Copyright (c) 2018 Jonathan Marek <jonathan@marek.ca>
+ *
+ * Based on msm_pwm_vibrator.c from downstream Android sources:
+ * Copyright (C) 2009-2014 LGE, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#define REG_CMD_RCGR 0x00
+#define REG_CFG_RCGR 0x04
+#define REG_M 0x08
+#define REG_N 0x0C
+#define REG_D 0x10
+#define REG_CBCR 0x24
+#define MMSS_CC_M_DEFAULT 1
+
+struct msm_vibrator {
+ struct input_dev *input;
+ struct mutex mutex;
+ struct work_struct worker;
+ void __iomem *base;
+ struct regulator *vcc;
+ struct clk *clk;
+ struct gpio_desc *enable_gpio;
+ u16 magnitude;
+ bool enabled;
+};
+
+static void msm_vibrator_write(struct msm_vibrator *vibrator, int offset,
+ u32 value)
+{
+ writel(value, vibrator->base + offset);
+}
+
+static int msm_vibrator_start(struct msm_vibrator *vibrator)
+{
+ int d_reg_val, ret = 0;
+
+ mutex_lock(&vibrator->mutex);
+
+ if (!vibrator->enabled) {
+ ret = clk_set_rate(vibrator->clk, 24000);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to set clock rate: %d\n", ret);
+ goto unlock;
+ }
+
+ ret = clk_prepare_enable(vibrator->clk);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to enable clock: %d\n", ret);
+ goto unlock;
+ }
+
+ ret = regulator_enable(vibrator->vcc);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to enable regulator: %d\n", ret);
+ clk_disable(vibrator->clk);
+ goto unlock;
+ }
+
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
+
+ vibrator->enabled = true;
+ }
+
+ d_reg_val = 127 - ((126 * vibrator->magnitude) / 0xffff);
+ msm_vibrator_write(vibrator, REG_CFG_RCGR,
+ (2 << 12) | /* dual edge mode */
+ (0 << 8) | /* cxo */
+ (7 << 0));
+ msm_vibrator_write(vibrator, REG_M, 1);
+ msm_vibrator_write(vibrator, REG_N, 128);
+ msm_vibrator_write(vibrator, REG_D, d_reg_val);
+ msm_vibrator_write(vibrator, REG_CMD_RCGR, 1);
+ msm_vibrator_write(vibrator, REG_CBCR, 1);
+
+unlock:
+ mutex_unlock(&vibrator->mutex);
+
+ return ret;
+}
+
+static void msm_vibrator_stop(struct msm_vibrator *vibrator)
+{
+ mutex_lock(&vibrator->mutex);
+
+ if (vibrator->enabled) {
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
+ regulator_disable(vibrator->vcc);
+ clk_disable(vibrator->clk);
+ vibrator->enabled = false;
+ }
+
+ mutex_unlock(&vibrator->mutex);
+}
+
+static void msm_vibrator_worker(struct work_struct *work)
+{
+ struct msm_vibrator *vibrator = container_of(work,
+ struct msm_vibrator,
+ worker);
+
+ if (vibrator->magnitude)
+ msm_vibrator_start(vibrator);
+ else
+ msm_vibrator_stop(vibrator);
+}
+
+static int msm_vibrator_play_effect(struct input_dev *dev, void *data,
+ struct ff_effect *effect)
+{
+ struct msm_vibrator *vibrator = input_get_drvdata(dev);
+
+ mutex_lock(&vibrator->mutex);
+
+ if (effect->u.rumble.strong_magnitude > 0)
+ vibrator->magnitude = effect->u.rumble.strong_magnitude;
+ else
+ vibrator->magnitude = effect->u.rumble.weak_magnitude;
+
+ mutex_unlock(&vibrator->mutex);
+
+ schedule_work(&vibrator->worker);
+
+ return 0;
+}
+
+static void msm_vibrator_close(struct input_dev *input)
+{
+ struct msm_vibrator *vibrator = input_get_drvdata(input);
+
+ cancel_work_sync(&vibrator->worker);
+ msm_vibrator_stop(vibrator);
+}
+
+static int msm_vibrator_probe(struct platform_device *pdev)
+{
+ struct msm_vibrator *vibrator;
+ struct resource *res;
+ int ret;
+
+ vibrator = devm_kzalloc(&pdev->dev, sizeof(*vibrator), GFP_KERNEL);
+ if (!vibrator)
+ return -ENOMEM;
+
+ vibrator->input = devm_input_allocate_device(&pdev->dev);
+ if (!vibrator->input)
+ return -ENOMEM;
+
+ vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
+ if (IS_ERR(vibrator->vcc)) {
+ if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
+ PTR_ERR(vibrator->vcc));
+ return PTR_ERR(vibrator->vcc);
+ }
+
+ vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(vibrator->enable_gpio)) {
+ if (PTR_ERR(vibrator->enable_gpio) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
+ PTR_ERR(vibrator->enable_gpio));
+ return PTR_ERR(vibrator->enable_gpio);
+ }
+
+ vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
+ if (IS_ERR(vibrator->clk)) {
+ if (PTR_ERR(vibrator->clk) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
+ PTR_ERR(vibrator->clk));
+ return PTR_ERR(vibrator->clk);
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get platform resource\n");
+ return -ENODEV;
+ }
+
+ vibrator->base = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
+ if (!vibrator->base) {
+ dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
+ PTR_ERR(vibrator->base));
+ return -ENOMEM;
+ }
+
+ vibrator->enabled = false;
+ mutex_init(&vibrator->mutex);
+ INIT_WORK(&vibrator->worker, msm_vibrator_worker);
+
+ vibrator->input->name = "msm-vibrator";
+ vibrator->input->id.bustype = BUS_HOST;
+ vibrator->input->close = msm_vibrator_close;
+
+ input_set_drvdata(vibrator->input, vibrator);
+ input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
+
+ ret = input_ff_create_memless(vibrator->input, NULL,
+ msm_vibrator_play_effect);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
+ return ret;
+ }
+
+ ret = input_register_device(vibrator->input);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register input device: %d", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, vibrator);
+
+ return 0;
+}
+
+static int __maybe_unused msm_vibrator_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
+
+ cancel_work_sync(&vibrator->worker);
+
+ if (vibrator->enabled)
+ msm_vibrator_stop(vibrator);
+
+ return 0;
+}
+
+static int __maybe_unused msm_vibrator_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
+
+ if (vibrator->enabled)
+ msm_vibrator_start(vibrator);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(msm_vibrator_pm_ops, msm_vibrator_suspend,
+ msm_vibrator_resume);
+
+static const struct of_device_id msm_vibrator_of_match[] = {
+ { .compatible = "qcom,msm8226-vibrator" },
+ { .compatible = "qcom,msm8974-vibrator" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, msm_vibrator_of_match);
+
+static struct platform_driver msm_vibrator_driver = {
+ .probe = msm_vibrator_probe,
+ .driver = {
+ .name = "msm-vibrator",
+ .pm = &msm_vibrator_pm_ops,
+ .of_match_table = of_match_ptr(msm_vibrator_of_match),
+ },
+};
+module_platform_driver(msm_vibrator_driver);
+
+MODULE_AUTHOR("Brian Masney <masneyb@onstation.org>");
+MODULE_DESCRIPTION("Qualcomm MSM vibrator driver");
+MODULE_LICENSE("GPL");
--
2.17.2
^ permalink raw reply related
* [PATCH v4 1/3] dt-bindings: Input: new bindings for MSM vibrator
From: Brian Masney @ 2019-02-06 1:33 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross
Cc: david.brown, robh+dt, mark.rutland, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-1-masneyb@onstation.org>
This patch adds the device tree bindings for the vibrator found on
various Qualcomm MSM SOCs.
Signed-off-by: Brian Masney <masneyb@onstation.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v3:
- None
.../bindings/input/msm-vibrator.txt | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/msm-vibrator.txt
diff --git a/Documentation/devicetree/bindings/input/msm-vibrator.txt b/Documentation/devicetree/bindings/input/msm-vibrator.txt
new file mode 100644
index 000000000000..8dcf014ef2e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/msm-vibrator.txt
@@ -0,0 +1,36 @@
+* Device tree bindings for the Qualcomm MSM vibrator
+
+Required properties:
+
+ - compatible: Should be one of
+ "qcom,msm8226-vibrator"
+ "qcom,msm8974-vibrator"
+ - reg: the base address and length of the IO memory for the registers.
+ - pinctrl-names: set to default.
+ - pinctrl-0: phandles pointing to pin configuration nodes. See
+ Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+ - clock-names: set to pwm
+ - clocks: phandle of the clock. See
+ Documentation/devicetree/bindings/clock/clock-bindings.txt
+ - enable-gpios: GPIO that enables the vibrator.
+
+Optional properties:
+
+ - vcc-supply: phandle to the regulator that provides power to the sensor.
+
+Example from a LG Nexus 5 (hammerhead) phone:
+
+vibrator@fd8c3450 {
+ reg = <0xfd8c3450 0x400>;
+ compatible = "qcom,msm8974-vibrator";
+
+ vcc-supply = <&pm8941_l19>;
+
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+ clock-names = "pwm";
+
+ enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&vibrator_pin>;
+};
--
2.17.2
^ permalink raw reply related
* [PATCH v4 0/3] ARM: qcom: add vibrator support for various MSM SOCs
From: Brian Masney @ 2019-02-06 1:33 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross
Cc: david.brown, robh+dt, mark.rutland, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
This patch set adds support for the vibrator found on various Qualcomm
MSM SOCs. This is based on work from:
Jonathan Marek from qcom,pwm-vibrator.c in the PostmarketOS repo:
https://gitlab.com/postmarketOS/linux-postmarketos/commit/7647fb36cb1cbd060f8b52087a68ab93583292b5
Jongrak Kwon and Devin Kim from msm_pwm_vibrator.c in the downstream
Android 3.4.0 sources:
https://android.googlesource.com/kernel/msm/+/android-msm-lenok-3.10-lollipop-wear-release/drivers/misc/msm_pwm_vibrator.c
Driver was tested on a LG Nexus 5 (hammerhead) phone using rumble-test:
https://git.collabora.com/cgit/user/sre/rumble-test.git/plain/rumble-test.c
Changes since v3
- Patch 2 is the only one that changed based on feedback from Dmitry
Torokhov. See notes on that patch for details.
Changes since v2
- Moved from pwm to input subsystem based on feedback from
https://lore.kernel.org/lkml/20181012114749.GC31561@ulmo/. I
previously wired this into the input system via pwm-vibra.
Changes since v1
- Update device tree binding document based on feedback from Rob
Herring.
- Changed the driver description to: Qualcomm PWM driver for the MSM
vibrator.
Brian Masney (3):
dt-bindings: Input: new bindings for MSM vibrator
Input: add new vibrator driver for various MSM SOCs
ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for
vibrator
.../bindings/input/msm-vibrator.txt | 36 +++
.../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 ++
drivers/input/misc/Kconfig | 10 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/msm-vibrator.c | 282 ++++++++++++++++++
5 files changed, 360 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/msm-vibrator.txt
create mode 100644 drivers/input/misc/msm-vibrator.c
--
2.17.2
^ permalink raw reply
* Re: [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs
From: Brian Masney @ 2019-02-06 0:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190205194246.GA19151@dtor-ws>
On Tue, Feb 05, 2019 at 11:42:46AM -0800, Dmitry Torokhov wrote:
> > + dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
> > + PTR_ERR(vibrator->clk));
> > + return PTR_ERR(vibrator->clk);
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get platform resource\n");
> > + return -ENODEV;
> > + }
> > +
> > + vibrator->base = devm_ioremap(&pdev->dev, res->start,
> > + resource_size(res));
> > + if (IS_ERR(vibrator->base)) {
>
> devm_ioremap() returns NULL on error. You either need to check for NULL
> or see of you can use devm_ioremap_resource().
I originally tried to use devm_ioremap_resource() but the call to
devm_request_mem_region() would fail. I'll go with the NULL check here
for devm_ioremap().
Thanks for the review!
Brian
^ permalink raw reply
* Re: [PATCH] Input: gpio-keys - Add shutdown callback
From: Dmitry Torokhov @ 2019-02-06 0:45 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, Jeffy Chen, Rob Herring, Andy Shevchenko,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...
In-Reply-To: <65d6350f-2cd8-c183-6d8b-dbb11ae31819@gmail.com>
On Tue, Feb 05, 2019 at 02:02:49PM -0800, Florian Fainelli wrote:
> On 2/1/19 11:24 AM, Florian Fainelli wrote:
> > On some platforms (e.g.: ARCH_BRCMSTB) it is possible to enter
> > "poweroff" while leaving some wake-up sources enabled such as key
> > presses in order to allow for the system to wake-up.
> >
> > Wire up a .shutdown() callback which calls into the existing
> > gpio_keys_suspend() since the logic is essentially the same.
> >
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/input/keyboard/gpio_keys.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
>
> Any feedback on this?
OK, I'll take it. Hopefully it won't affect existing systems as the
change is unconditional.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: stmfts - acknowledge that setting brightness is a blocking call
From: Dmitry Torokhov @ 2019-02-05 22:46 UTC (permalink / raw)
To: linux-input; +Cc: Andi Shyti, Jacek Anaszewski, linux-kernel
We need to turn regulators on and off when switching brightness, and
that may block, therefore we have to set stmfts_brightness_set() as
LED's brightness_set_blocking() method.
Fixes: 78bcac7b2ae1 ("Input: add support for the STMicroelectronics FingerTip touchscreen")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/stmfts.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/stmfts.c b/drivers/input/touchscreen/stmfts.c
index 704e99046916..b6f95f20f924 100644
--- a/drivers/input/touchscreen/stmfts.c
+++ b/drivers/input/touchscreen/stmfts.c
@@ -106,27 +106,29 @@ struct stmfts_data {
bool running;
};
-static void stmfts_brightness_set(struct led_classdev *led_cdev,
+static int stmfts_brightness_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
struct stmfts_data *sdata = container_of(led_cdev,
struct stmfts_data, led_cdev);
int err;
- if (value == sdata->led_status || !sdata->ledvdd)
- return;
-
- if (!value) {
- regulator_disable(sdata->ledvdd);
- } else {
- err = regulator_enable(sdata->ledvdd);
- if (err)
- dev_warn(&sdata->client->dev,
- "failed to disable ledvdd regulator: %d\n",
- err);
+ if (value != sdata->led_status && sdata->ledvdd) {
+ if (!value) {
+ regulator_disable(sdata->ledvdd);
+ } else {
+ err = regulator_enable(sdata->ledvdd);
+ if (err) {
+ dev_warn(&sdata->client->dev,
+ "failed to disable ledvdd regulator: %d\n",
+ err);
+ return err;
+ }
+ }
+ sdata->led_status = value;
}
- sdata->led_status = value;
+ return 0;
}
static enum led_brightness stmfts_brightness_get(struct led_classdev *led_cdev)
@@ -608,7 +610,7 @@ static int stmfts_enable_led(struct stmfts_data *sdata)
sdata->led_cdev.name = STMFTS_DEV_NAME;
sdata->led_cdev.max_brightness = LED_ON;
sdata->led_cdev.brightness = LED_OFF;
- sdata->led_cdev.brightness_set = stmfts_brightness_set;
+ sdata->led_cdev.brightness_set_blocking = stmfts_brightness_set;
sdata->led_cdev.brightness_get = stmfts_brightness_get;
err = devm_led_classdev_register(&sdata->client->dev, &sdata->led_cdev);
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: cap11xx - switch to using set_brightness_blocking()
From: Dmitry Torokhov @ 2019-02-05 22:20 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, Jacek Anaszewski, linux-kernel
Updating LED state requires access to regmap and therefore we may sleep, so
we could not do that directly form set_brightness() method. Historically
we used private work to adjust the brightness, but with the introduction of
set_brightness_blocking() we no longer need it.
As a bonus, not having our own work item means we do not have
use-after-free issue as we neglected to cancel outstanding work on driver
unbind.
Reported-by: Sven Van Asbroeck <thesven73@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/cap11xx.c | 47 ++++++++++++++------------------
1 file changed, 21 insertions(+), 26 deletions(-)
diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index 312916f99597..c0baf323ddda 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -75,9 +75,8 @@
struct cap11xx_led {
struct cap11xx_priv *priv;
struct led_classdev cdev;
- struct work_struct work;
u32 reg;
- enum led_brightness new_brightness;
+ enum led_brightness brightness;
};
#endif
@@ -233,30 +232,28 @@ static void cap11xx_input_close(struct input_dev *idev)
}
#ifdef CONFIG_LEDS_CLASS
-static void cap11xx_led_work(struct work_struct *work)
-{
- struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
- struct cap11xx_priv *priv = led->priv;
- int value = led->new_brightness;
-
- /*
- * All LEDs share the same duty cycle as this is a HW limitation.
- * Brightness levels per LED are either 0 (OFF) and 1 (ON).
- */
- regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
- BIT(led->reg), value ? BIT(led->reg) : 0);
-}
-
-static void cap11xx_led_set(struct led_classdev *cdev,
- enum led_brightness value)
+static int cap11xx_led_set(struct led_classdev *cdev,
+ enum led_brightness value)
{
struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
+ struct cap11xx_priv *priv = led->priv;
+ int error = 0;
+
+ if (led->brightness != value) {
+ /*
+ * All LEDs share the same duty cycle as this is a HW
+ * limitation. Brightness levels per LED are either
+ * 0 (OFF) and 1 (ON).
+ */
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_LED_OUTPUT_CONTROL,
+ BIT(led->reg),
+ value ? BIT(led->reg) : 0);
+ if (!error)
+ led->brightness = value;
+ }
- if (led->new_brightness == value)
- return;
-
- led->new_brightness = value;
- schedule_work(&led->work);
+ return error;
}
static int cap11xx_init_leds(struct device *dev,
@@ -299,7 +296,7 @@ static int cap11xx_init_leds(struct device *dev,
led->cdev.default_trigger =
of_get_property(child, "linux,default-trigger", NULL);
led->cdev.flags = 0;
- led->cdev.brightness_set = cap11xx_led_set;
+ led->cdev.brightness_set_blocking = cap11xx_led_set;
led->cdev.max_brightness = 1;
led->cdev.brightness = LED_OFF;
@@ -312,8 +309,6 @@ static int cap11xx_init_leds(struct device *dev,
led->reg = reg;
led->priv = priv;
- INIT_WORK(&led->work, cap11xx_led_work);
-
error = devm_led_classdev_register(dev, &led->cdev);
if (error) {
of_node_put(child);
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: gpio-keys - Add shutdown callback
From: Florian Fainelli @ 2019-02-05 22:02 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Torokhov, Jeffy Chen, Rob Herring, Andy Shevchenko,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...
In-Reply-To: <20190201192417.15037-1-f.fainelli@gmail.com>
On 2/1/19 11:24 AM, Florian Fainelli wrote:
> On some platforms (e.g.: ARCH_BRCMSTB) it is possible to enter
> "poweroff" while leaving some wake-up sources enabled such as key
> presses in order to allow for the system to wake-up.
>
> Wire up a .shutdown() callback which calls into the existing
> gpio_keys_suspend() since the logic is essentially the same.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/input/keyboard/gpio_keys.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Any feedback on this?
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 492a971b95b5..6cd199e8a370 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -1015,8 +1015,18 @@ static int __maybe_unused gpio_keys_resume(struct device *dev)
>
> static SIMPLE_DEV_PM_OPS(gpio_keys_pm_ops, gpio_keys_suspend, gpio_keys_resume);
>
> +static void gpio_keys_shutdown(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = gpio_keys_suspend(&pdev->dev);
> + if (ret)
> + dev_err(&pdev->dev, "failed to shutdown\n");
> +}
> +
> static struct platform_driver gpio_keys_device_driver = {
> .probe = gpio_keys_probe,
> + .shutdown = gpio_keys_shutdown,
> .driver = {
> .name = "gpio-keys",
> .pm = &gpio_keys_pm_ops,
>
--
Florian
^ permalink raw reply
* Re: [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs
From: Dmitry Torokhov @ 2019-02-05 19:42 UTC (permalink / raw)
To: Brian Masney
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20181025012937.2154-3-masneyb@onstation.org>
Hi Brian,
On Wed, Oct 24, 2018 at 09:29:36PM -0400, Brian Masney wrote:
> This patch adds a new vibrator driver that supports various Qualcomm
> MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
>
...
> +
> +#define msm_vibrator_write(msm_vibrator, offset, value) \
> + writel((value), (void __iomem *)((msm_vibrator)->base + (offset)))
Make in a function? It will be inlined anyways...
> +
> + vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
> + if (IS_ERR(vibrator->vcc)) {
> + if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
> + PTR_ERR(vibrator->vcc));
> + return PTR_ERR(vibrator->vcc);
> + }
> +
> + vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(vibrator->enable_gpio)) {
You have explicit deferral handling for the regulator, but not gpio. I'd
prefer if we did (or not) it consistently.
> + dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
> + PTR_ERR(vibrator->enable_gpio));
> + return PTR_ERR(vibrator->enable_gpio);
> + }
> +
> + vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(vibrator->clk)) {
Same here.
> + dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
> + PTR_ERR(vibrator->clk));
> + return PTR_ERR(vibrator->clk);
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get platform resource\n");
> + return -ENODEV;
> + }
> +
> + vibrator->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (IS_ERR(vibrator->base)) {
devm_ioremap() returns NULL on error. You either need to check for NULL
or see of you can use devm_ioremap_resource().
> + dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
> + PTR_ERR(vibrator->base));
> + return PTR_ERR(vibrator->base);
> + }
> +
> + vibrator->enabled = false;
> + mutex_init(&vibrator->mutex);
> + INIT_WORK(&vibrator->worker, msm_vibrator_worker);
> +
> + vibrator->input->name = "msm-vibrator";
> + vibrator->input->id.bustype = BUS_HOST;
> + vibrator->input->dev.parent = &pdev->dev;
You allocated input device with devm so there is no need to set parent
by hand.
> + vibrator->input->close = msm_vibrator_close;
> +
> + input_set_drvdata(vibrator->input, vibrator);
> + input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
> +
> + ret = input_ff_create_memless(vibrator->input, NULL,
> + msm_vibrator_play_effect);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
> + return ret;
> + }
> +
> + ret = input_register_device(vibrator->input);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register input device: %d", ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_suspend(struct device *dev)
> +{
> + struct msm_vibrator *vibrator = dev_get_drvdata(dev);
Prefer explicit platform_get_drvdata() since we are working with
platform device (even though it resolves to the same thing).
> +
> + cancel_work_sync(&vibrator->worker);
> +
> + if (vibrator->enabled)
> + msm_vibrator_stop(vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_resume(struct device *dev)
> +{
> + struct msm_vibrator *vibrator = dev_get_drvdata(dev);
Same here.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] platform/x86: silead_dmi: Add touchscreen platform data for the Chuwi Hi8 Air tablet
From: Andy Shevchenko @ 2019-02-05 18:18 UTC (permalink / raw)
To: Kai Renzig
Cc: Hans de Goede, Darren Hart, Andy Shevchenko, linux-input,
Platform Driver, Linux Kernel Mailing List
In-Reply-To: <20190203183423.7661-1-k.renzig@gmail.com>
On Sun, Feb 3, 2019 at 8:34 PM Kai Renzig <k.renzig@gmail.com> wrote:
>
> Add touchscreen platform data for the Chuwi Hi8 Air tablet.
>
Pushed to my review and testing queue, thanks!
> Signed-off-by: Kai Renzig <k.renzig@gmail.com>
> ---
> Changes in v2:
> - Fix the firmware filename to match the actual touchscreen controller.
>
> drivers/platform/x86/touchscreen_dmi.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 8c5d47c0aea6..1f66405928a9 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -41,6 +41,20 @@ static const struct ts_dmi_data chuwi_hi8_data = {
> .properties = chuwi_hi8_props,
> };
>
> +static const struct property_entry chuwi_hi8_air_props[] = {
> + PROPERTY_ENTRY_U32("touchscreen-size-x", 1728),
> + PROPERTY_ENTRY_U32("touchscreen-size-y", 1148),
> + PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
> + PROPERTY_ENTRY_STRING("firmware-name", "gsl3676-chuwi-hi8-air.fw"),
> + PROPERTY_ENTRY_U32("silead,max-fingers", 10),
> + { }
> +};
> +
> +static const struct ts_dmi_data chuwi_hi8_air_data = {
> + .acpi_name = "MSSL1680:00",
> + .properties = chuwi_hi8_air_props,
> +};
> +
> static const struct property_entry chuwi_hi8_pro_props[] = {
> PROPERTY_ENTRY_U32("touchscreen-min-x", 6),
> PROPERTY_ENTRY_U32("touchscreen-min-y", 3),
> @@ -497,6 +511,15 @@ static const struct dmi_system_id touchscreen_dmi_table[] = {
> DMI_MATCH(DMI_BIOS_VERSION, "H1D_S806_206"),
> },
> },
> + {
> + /* Chuwi Hi8 Air (CWI543) */
> + .driver_data = (void *)&chuwi_hi8_air_data,
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Default string"),
> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Hi8 Air"),
> + },
> + },
> {
> /* Chuwi Hi8 Pro (CWI513) */
> .driver_data = (void *)&chuwi_hi8_pro_data,
> --
> 2.19.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-02-05 15:32 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Andy Shevchenko, Dmitry Torokhov, Henrik Rydberg, Lukas Wunner,
Federico Lorenzi, linux-input, Linux Kernel Mailing List
In-Reply-To: <20190205131812.GC4225@innovation.ch>
On Tue, Feb 5, 2019 at 4:21 PM Life is hard, and then you die
<ronald@innovation.ch> wrote:
> > > +config KEYBOARD_APPLESPI
> > > + tristate "Apple SPI keyboard and trackpad"
> >
> > > + depends on (X86 && ACPI && SPI) || COMPILE_TEST
> >
> > COMPILE_TEST more or less makes sense in conjunction with architecture selection.
> > It means, your code always dependant to ACPI and SPI frameworks.
> > That's why 0day complained.
>
> Thanks. Yes, looking at this again I realized I somewhat misunderstood
> the uses of COMPILE_TEST. I've changed this now to
>
> depends on ACPI && SPI && (X86 || COMPILE_TEST)
Better to split like
depends on SPI && ACPI
depends on X86 || COMPILE_TEST
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Life is hard, and then you die @ 2019-02-05 13:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190205114522.GV9224@smile.fi.intel.com>
Hi Andy,
On Tue, Feb 05, 2019 at 01:45:22PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
> >
> > In order for this driver to work, the proper SPI drivers need to be
> > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > reason enabling this driver in the config implies enabling the above
> > drivers.
>
> > +config KEYBOARD_APPLESPI
> > + tristate "Apple SPI keyboard and trackpad"
>
> > + depends on (X86 && ACPI && SPI) || COMPILE_TEST
>
> COMPILE_TEST more or less makes sense in conjunction with architecture selection.
> It means, your code always dependant to ACPI and SPI frameworks.
> That's why 0day complained.
Thanks. Yes, looking at this again I realized I somewhat misunderstood
the uses of COMPILE_TEST. I've changed this now to
depends on ACPI && SPI && (X86 || COMPILE_TEST)
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Life is hard, and then you die @ 2019-02-05 13:15 UTC (permalink / raw)
To: Dan Carpenter
Cc: kbuild, kbuild-all, Dmitry Torokhov, Henrik Rydberg, Lukas Wunner,
Federico Lorenzi, Andy Shevchenko, linux-input, linux-kernel
In-Reply-To: <20190205102110.GG2581@kadam>
Hi Dan,
On Tue, Feb 05, 2019 at 01:21:10PM +0300, Dan Carpenter wrote:
> Hi Ronald,
>
> Thank you for the patch! Perhaps something to improve:
>
> url: https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
> base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
>
> smatch warnings:
> drivers/input/keyboard/applespi.c:1551 applespi_get_saved_bl_level() warn: returning -1 instead of -ENOMEM is sloppy
[snip]
> cfa9f3705 Ronald Tschalär 2019-02-04 1549 efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
> cfa9f3705 Ronald Tschalär 2019-02-04 1550 if (!efivar_entry)
> cfa9f3705 Ronald Tschalär 2019-02-04 @1551 return -1;
Agreed, that is sloppy. Fixed in for next version. Thanks!
Cheers,
Ronald
^ 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