* Re: [PATCH v5 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Kamel Bouhara @ 2024-01-25 16:07 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, catalin.popescu, mark.satterthwaite, bartp,
hannah.rossiter, Thomas Petazzoni, Gregory Clement,
bsp-development.geo
In-Reply-To: <ZY5An58Rffrcpfpn@nixie71>
On Thu, Dec 28, 2023 at 09:44:31PM -0600, Jeff LaBundy wrote:
> Hi Kamel,
>
Hello Jeff,
Thanks for your review again and sorry for those late answers below.
...
> +#include <linux/module.h>
> > +#include <linux/of.h>
>
> As I mention in the v3 review, the entire of.h is not necessary in the
> case of this driver; mod_devicetable.h is sufficient. Please see:
>
> dbce1a7d5dce ("Input: Explicitly include correct DT includes")
>
Ok.
> > +
> > +#define AXIOM_PROX_LEVEL -128
> > +/*
> > + * Register group u31 has 2 pages for usage table entries.
> > + */
> > +#define AXIOM_U31_MAX_USAGES ((2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE)
> > +#define AXIOM_U31_BYTES_PER_USAGE 6
> > +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> > +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> > +#define AXIOM_U31_DEVID_MASK GENMASK(14, 0)
> > +
> > +#define AXIOM_CMD_HEADER_READ_MASK BIT(15)
> > +#define AXIOM_U41_MAX_TARGETS 10
> > +
> > +#define AXIOM_U46_AUX_CHANNELS 4
> > +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
> > +
> > +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> > +#define AXIOM_COMMS_PAGE_SIZE 256
> > +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(6, 0)
> > +
> > +#define AXIOM_REPORT_USAGE_ID 0x34
> > +#define AXIOM_DEVINFO_USAGE_ID 0x31
> > +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> > +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> > +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> > +
> > +#define AXIOM_PAGE_OFFSET_MASK GENMASK(6, 0)
> > +
> > +struct axiom_devinfo {
> > + u16 device_id;
>
> Assuming this is a packed struct into which data is directly read over
> I2C, this member needs declared as __be16 or __le16 depending on the
> endianness of the device, and then all accesses to it resolved using
> be16_to_cpu() or le16_to_cpu().
>
> > + u8 fw_minor;
> > + u8 fw_major;
> > + u8 fw_info_extra;
> > + u8 tcp_revision;
> > + u8 bootloader_fw_minor;
> > + u8 bootloader_fw_major;
> > + u16 jedec_id;
>
> And here.
>
Ack and applied to v6, thanks.
> > + u8 num_usages;
> > +} __packed;
> > +
> > +/*
> > + * Describes parameters of a specific usage, essentially a single element of
> > + * the "Usage Table"
> > + */
> > +struct axiom_usage_entry {
> > + u8 id;
> > + u8 is_report;
> > + u8 start_page;
> > + u8 num_pages;
> > +};
> > +
> > +/*
> > + * Represents state of a touch or target when detected prior a touch (eg.
> > + * hover or proximity events).
> > + */
>
> Nit: prior to a touch
>
Fixed.
> > +enum axiom_target_state {
> > + AXIOM_TARGET_STATE_NOT_PRESENT = 0,
> > + AXIOM_TARGET_STATE_PROX = 1,
> > + AXIOM_TARGET_STATE_HOVER = 2,
> > + AXIOM_TARGET_STATE_TOUCHING = 3,
> > +};
> > +
> > +struct axiom_u41_target {
> > + enum axiom_target_state state;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > + bool insert;
> > + bool touch;
> > +};
> > +
> > +struct axiom_target_report {
> > + u8 index;
> > + u8 present;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > +};
> > +
> > +struct axiom_cmd_header {
> > + __le16 target_address;
> > + __le16 length;
> > +} __packed;
> > +
> > +struct axiom_data {
> > + struct axiom_devinfo devinfo;
> > + struct device *dev;
> > + struct gpio_desc *reset_gpio;
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + u32 max_report_len;
> > + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
>
> Please use standard kernel type definitions (e.g. u8).
Applied.
>
> > + struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
> > + struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> > + bool usage_table_populated;
> > + struct regulator *vdda;
> > + struct regulator *vddi;
> > +};
> > +
> > +/*
> > + * axiom devices are typically configured to report
> > + * touches at a rate of 100Hz (10ms). For systems
> > + * that require polling for reports.
>
> It's not entirely clear what this is saying; is the first period meant to be
> replaced with a comma? It's also odd to see some comments limited to half the
> column width of others. Please make another pass through these to give the
> commentary a consistent voice.
I believe second sentence is not required indeed, fixed the column
width as well, thanks.
>
> > + * When reports are polled, it will be expected to
> > + * occasionally observe the overflow bit being set
> > + * in the reports. This indicates that reports are not
> > + * being read fast enough.
> > + */
> > +#define POLL_INTERVAL_DEFAULT_MS 10
> > +
> > +/* Translate usage/page/offset triplet into physical address. */
> > +static u16 axiom_usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > + char offset)
> > +{
> > + u32 i;
>
> It's more common in kernel code for iterators to be declared as 'int' than
> u32, even if they're only used as unsigned integers in this case.
>
Ack.
[...]
> + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
>
> My comment here from v3 seems to have been missed; please check it.
>
Applied to v6.
> > +
> > + error = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (error != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev,
> > + "Failed reading usage %#x page %#x, error=%d\n",
> > + usage, page, error);
> > + return -EIO;
> > + }
>
> As I mention in the v3 review, you should preserve the original error code
> in case of a negative return value instead of returning -EIO in all cases.
> Please check my original comment.
>
> I also recommend you call this 'ret' and not 'error', because a non-zero
> return value (2) actually indicates success. In the input subsystem at least,
> 'error' is typically used for return values that can only be zero or negative.
>
> > +
> > + usleep_range(250, 300);
> > +
> > + return 0;
> > +}
>
> During the v3 review, I suggested you use regmap, since SPI support may come
> later. You can override both I2C and SPI callbacks with your own in case the
> hardware requires it. What was the reason not to use regmap now, and minimize
> rip-up later?
Suggestion applied to v6.
>
> > +
> > +/*
> > + * One of the main purposes for reading the usage table is to identify
> > + * which usages reside at which target address.
> > + * When performing subsequent reads or writes to AXIOM, the target address
> > + * is used to specify which usage is being accessed.
> > + * Consider the following discovery code which will build up the usage table.
> > + */
> > +static u32 axiom_populate_usage_table(struct axiom_data *ts)
> > +{
> > + struct axiom_usage_entry *usage_table;
> > + u32 max_report_len = 0;
> > + char *rx_data = ts->rx_buf;
>
> Please use u8 here.
>
> > + u32 usage_id;
> > + int error;
> > +
> > + usage_table = ts->usage_table;
> > +
> > + /* Read the second page of usage u31 to get the usage table */
> > + error = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> > + (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> > + if (error)
> > + return error;
> > +
> > + for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
> > + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> > + u8 id = rx_data[offset + 0];
> > + u8 start_page = rx_data[offset + 1];
> > + u8 num_pages = rx_data[offset + 2];
> > + u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> > +
> > + if (!num_pages)
> > + usage_table[usage_id].is_report = true;
> > +
> > + /* Store the entry into the usage table */
> > + usage_table[usage_id].id = id;
> > + usage_table[usage_id].start_page = start_page;
> > + usage_table[usage_id].num_pages = num_pages;
> > +
> > + dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id,
> > + AXIOM_U31_BYTES_PER_USAGE, &rx_data[offset]);
> > +
> > + /* Identify the max report length the module will receive */
> > + if (usage_table[usage_id].is_report && max_offset > max_report_len)
> > + max_report_len = max_offset;
> > + }
> > +
> > + ts->usage_table_populated = true;
> > +
> > + return max_report_len;
> > +}
> > +
> > +static int axiom_discover(struct axiom_data *ts)
> > +{
> > + int error;
> > +
> > + /*
> > + * Fetch the first page of usage u31 to get the
> > + * device information and the number of usages
> > + */
> > + error = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, (char *)&ts->devinfo,
> > + AXIOM_U31_PAGE0_LENGTH);
>
> If you're set on using bespoke I2C helpers instead of regmap, then 'buf'
> should be defined as a void * as opposed to casting outside of axiom_i2c_read().
>
> > + if (error)
> > + return error;
> > +
> > + dev_dbg(ts->dev, " Boot Mode : %s\n",
> > + FIELD_GET(AXIOM_U31_BOOTMODE_MASK, ts->devinfo.device_id) ? "BLP" : "TCP");
> > + dev_dbg(ts->dev, " Device ID : %04lx\n",
> > + FIELD_GET(AXIOM_U31_DEVID_MASK, ts->devinfo.device_id));
> > + dev_dbg(ts->dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> > + ts->devinfo.fw_minor);
> > + dev_dbg(ts->dev, " Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
> > + ts->devinfo.bootloader_fw_minor);
> > + dev_dbg(ts->dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> > + dev_dbg(ts->dev, " Silicon : %04x\n", ts->devinfo.jedec_id);
> > + dev_dbg(ts->dev, " Number usages : %04x\n", ts->devinfo.num_usages);
> > +
> > + ts->max_report_len = axiom_populate_usage_table(ts);
> > + if (!ts->max_report_len || !ts->devinfo.num_usages)
>
> This seems worthy of a dev_err(), otherwise the customer has no way to
> know something is wrong with the controller's FW.
>
> > + return -EINVAL;
> > +
> > + dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Support function to axiom_process_u41_report.
> > + * Generates input-subsystem events for every target.
> > + * After calling this function the caller shall issue
> > + * a Sync to the input sub-system.
> > + */
> > +static bool axiom_process_u41_report_target(struct axiom_data *ts,
> > + struct axiom_target_report *target)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + struct axiom_u41_target *target_prev_state;
> > + enum axiom_target_state current_state;
> > + bool update = false;
> > + int slot;
> > +
> > + /* Verify the target index */
> > + if (target->index >= AXIOM_U41_MAX_TARGETS) {
> > + dev_dbg(ts->dev, "Invalid target index! %u\n", target->index);
>
> Should this be dev_err()?
>
> > + return false;
> > + }
> > +
> > + target_prev_state = &ts->targets[target->index];
> > +
> > + current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
> > +
> > + if (target->present) {
> > + if (target->z >= 0)
> > + current_state = AXIOM_TARGET_STATE_TOUCHING;
> > + else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> > + current_state = AXIOM_TARGET_STATE_HOVER;
> > + else if (target->z == AXIOM_PROX_LEVEL)
> > + current_state = AXIOM_TARGET_STATE_PROX;
> > + }
> > +
> > + if (target_prev_state->state == current_state &&
> > + target_prev_state->x == target->x &&
> > + target_prev_state->y == target->y &&
> > + target_prev_state->z == target->z) {
> > + return false;
> > + }
>
> No need for curly braces here; please refer to the kernel style guidelines.
>
> > +
> > + slot = target->index;
> > +
> > + dev_dbg(ts->dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
> > + target->index, slot, target->present,
> > + target->x, target->y, target->z);
> > +
> > + switch (current_state) {
> > + case AXIOM_TARGET_STATE_NOT_PRESENT:
> > + case AXIOM_TARGET_STATE_PROX:
> > + if (!target_prev_state->insert)
> > + break;
> > + update = true;
> > + target_prev_state->insert = false;
> > + input_mt_slot(input_dev, slot);
> > +
> > + if (!slot)
> > + input_report_key(input_dev, BTN_TOUCH, 0);
> > +
> > + input_mt_report_slot_inactive(input_dev);
> > + /*
> > + * make sure the previous coordinates are
> > + * all off screen when the finger comes back
> > + */
> > + target->x = 65535;
> > + target->y = 65535;
> > + target->z = AXIOM_PROX_LEVEL;
> > + break;
> > + case AXIOM_TARGET_STATE_HOVER:
> > + case AXIOM_TARGET_STATE_TOUCHING:
> > + target_prev_state->insert = true;
> > + update = true;
> > + input_mt_slot(input_dev, slot);
> > + input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
> > + input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> > + input_report_abs(input_dev, ABS_X, target->x);
>
> You do not need to explicitly report ABS_X and ABS_Y values, as calling
> input_mt_sync_frame() effectively takes care of this by way of pointer
> emulation.
Applied thanks.
>
> > + input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> > + input_report_abs(input_dev, ABS_Y, target->y);
> > +
> > + if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
> > + input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> > + input_report_abs(input_dev, ABS_DISTANCE, 0);
> > + input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> > + input_report_abs(input_dev, ABS_PRESSURE, target->z);
> > + } else {
> > + input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> > + input_report_abs(input_dev, ABS_DISTANCE, -target->z);
> > + input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> > + input_report_abs(input_dev, ABS_PRESSURE, 0);
> > + }
> > +
> > + if (!slot)
> > + input_report_key(input_dev, BTN_TOUCH, (current_state ==
> > + AXIOM_TARGET_STATE_TOUCHING));
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + target_prev_state->state = current_state;
> > + target_prev_state->x = target->x;
> > + target_prev_state->y = target->y;
> > + target_prev_state->z = target->z;
> > +
> > + return update;
> > +}
>
> I appreciate that some clean-up was done here, but it still seems you can
> get rid of the 'update' flag. Can you not re-shuffle this a bit so that
> you return true at the bottom of the function, and simply return false
> early for the few cases where there is no update?
>
Sure.
> > +
> > +/*
> > + * U41 is the output report of the 2D CTS and contains the status of targets
> > + * (including contacts and pre-contacts) along with their X,Y,Z values.
> > + * When a target has been removed (no longer detected),
> > + * the corresponding X,Y,Z values will be zeroed.
> > + */
> > +static bool axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct axiom_target_report target;
> > + bool update_done = false;
> > + u16 target_status;
> > + u32 i;
> > +
> > + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
>
> Please use get_unaligned_le16() instead of open-coding this math.
Interesting, thanks for the suggestions that will indeed improve
readability.
>
> > +
> > + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> > + char target_step = i * 4;
>
> Please use u8 here.
Ack.
>
> > +
> > + target.index = i;
> > + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> > + target.x = (rx_buf[(target_step + 3)] | (rx_buf[target_step + 4] << 8));
> > + target.y = (rx_buf[(target_step + 5)] | (rx_buf[target_step + 6] << 8));
> > + target.z = (s8)(rx_buf[i + 43]);
> > + update_done |= axiom_process_u41_report_target(ts, &target);
> > + }
> > +
> > + return update_done;
> > +}
> > +
> > +/*
> > + * U46 report contains a low level measurement data generated by the CDS
> > + * algorithms from the AUX channels. This information is useful when tuning
> > + * multi-press to assess mechanical consistency in the unit's construction.
> > + */
>
> What does CDS stand for, and what in user space is interested in these
> events? I'm guessing some kind of production-line calibration tool? I
> appreciate the additional comments in this revision; please add a bit
> more here.
CDS here stands for Capacitive Displacement Sensor and I think you are
right about the calibration tool.
>
> > +static void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + u32 event_value;
> > + u16 aux_value;
> > + u32 i = 0;
>
> There is no need to initialize this iterator.
>
Fixed, thanks.
> > +
> > + for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
> > + char target_step = i * 2;
> > +
> > + aux_value = ((rx_buf[target_step + 2] << 8) | (rx_buf[target_step + 1]))
> > + & AXIOM_U46_AUX_MASK;
>
> This looks like another opportunity to use get_unaligned_le16().
>
Ack.
> > + event_value = (i << 16) | (aux_value);
> > + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> > + }
> > +}
> > +
> > +/*
> > + * Validates the crc and demultiplexes the axiom reports to the appropriate
> > + * report handler
> > + */
> > +static int axiom_handle_events(struct axiom_data *ts)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + char *report_data = ts->rx_buf;
>
> Please use u8 here.
>
> > + struct device *dev = ts->dev;
> > + u16 crc_report;
> > + u16 crc_calc;
> > + int error;
> > + char len;
>
> And here.
>
> > +
> > + error = axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data,
> > + ts->max_report_len);
> > + if (error)
> > + return error;
> > +
> > + len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
> > + if (!len) {
> > + dev_err(dev, "Zero length report discarded.\n");
> > + return -ENODATA;
> > + }
>
> Since you're expecting at least two bytes to get a CRC, it seems you should
> check that len >= 2 instead of > 0, otherwise 'len - 2' below will panic.
>
> > +
> > + /* Validate the report CRC */
> > + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
>
> We can use get_unaligned_le16() here too.
>
> > + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> > + crc_calc = crc16(0, report_data, (len - 2));
> > +
> > + if (crc_calc != crc_report) {
> > + dev_err(dev,
> > + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> > + crc_report, crc_calc);
> > + return -EINVAL;
> > + }
> > +
> > + switch (report_data[1]) {
> > + case AXIOM_USAGE_2DCTS_REPORT_ID:
> > + if (axiom_process_u41_report(ts, &report_data[1])) {
> > + input_mt_sync_frame(input_dev);
> > + input_sync(input_dev);
> > + }
> > + break;
> > +
> > + case AXIOM_USAGE_2AUX_REPORT_ID:
> > + /* This is an aux report (force) */
> > + axiom_process_u46_report(ts, &report_data[1]);
> > + input_mt_sync(input_dev);
>
> This call to input_mt_sync() seems unnecessary; we are not touching any MT
> slots in this case.
>
> > + input_sync(input_dev);
> > + break;
> > +
> > + case AXIOM_USAGE_2HB_REPORT_ID:
> > + /* This is a heartbeat report */
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_poll(struct input_dev *input_dev)
> > +{
> > + struct axiom_data *ts = input_get_drvdata(input_dev);
> > +
> > + axiom_handle_events(ts);
> > +}
> > +
> > +static irqreturn_t axiom_irq(int irq, void *dev_id)
> > +{
> > + struct axiom_data *ts = dev_id;
> > +
> > + axiom_handle_events(ts);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void axiom_reset(struct gpio_desc *reset_gpio)
> > +{
> > + gpiod_set_value_cansleep(reset_gpio, 1);
> > + usleep_range(1000, 2000);
> > + gpiod_set_value_cansleep(reset_gpio, 0);
> > + msleep(110);
> > +}
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct input_dev *input_dev;
> > + struct axiom_data *ts;
> > + u32 startup_delay_ms;
> > + u32 poll_interval;
> > + int target;
> > + int error;
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > + ts->dev = dev;
> > +
> > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ts->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
> > +
> > + if (ts->reset_gpio)
> > + axiom_reset(ts->reset_gpio);
> > +
> > + ts->vddi = devm_regulator_get_optional(dev, "VDDI");
>
> I don't think there is any rule against doing so, but I have never seen any
> customers name a regulator in all caps.
>
> > + if (!IS_ERR(ts->vddi)) {
> > + error = regulator_enable(ts->vddi);
> > + if (error)
> > + return dev_err_probe(&client->dev, error,
> > + "Failed to enable VDDI regulator\n");
> > + }
> > +
> > + ts->vdda = devm_regulator_get_optional(dev, "VDDA");
> > + if (!IS_ERR(ts->vdda)) {
> > + error = regulator_enable(ts->vdda);
> > + if (error) {
> > + dev_err(dev, "Failed to get VDDA regulator\n");
> > + regulator_disable(ts->vddi);
>
> You're turning off VDDI in case VDDA fails to be enabled, but you never turn
> either off in case the rest of probe (e.g. I2C read) fails, or any other time
> for that matter. Please schedule the regulator_disable() calls using
> devm_add_action_or_reset() so that they are automatically disabled in sequence
> in case probe terminates early, or the driver is unloaded.
I fixed this just by using devm_regulator_get_enable() instead, this
should also make sure regulators ar disabled in case of probe issue.
>
> > + return error;
> > + }
> > + if (!device_property_read_u32(dev, "startup-time-ms", &startup_delay_ms))
> > + msleep(startup_delay_ms);
>
> This seems like it should be a constraint handled by the regulator core and
> not your driver.
Not sure this fits well in regulator's ramp-delay feature, this delay is
required only after the regulator is enabled.
>
> > + }
> > +
> > + error = axiom_discover(ts);
> > + if (error)
> > + return dev_err_probe(dev, error, "Failed touchscreen discover\n");
> > +
> > + input_dev = devm_input_allocate_device(ts->dev);
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + input_dev->name = "TouchNetix axiom Touchscreen";
> > + input_dev->phys = "input/axiom_ts";
> > +
> > + /* Single Touch */
> > + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
>
> As I explained in the v3 review, you do not need to do this. Please refer to my
> previous comments.
>
> > +
> > + /* Multi Touch */
>
> This comment is unnecessary.
>
Fixed in v6.
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
>
> What is the point of this comment, and the one below? Should fuzz have been 4?
>
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
>
> It seems you are forcing 65535 by 65535 resolution; is there no way to
> adjust this? Most controllers can scale it in their FW. You should either
> accept a customer-defined resolution using touchscreen_parse_properties()
> and write it through to the FW, read it from FW and report it through
> input_set_abs_params(), or both.
Ack, I will take this as well, thanks.
>
> > +
> > + input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
>
> This is unnecessary; input_mt_init_slots() will do it based on the contact type.
>
> > +
> > + /* Registers the axiom device as a touchscreen instead of a mouse pointer */
> > + error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > + if (error)
> > + return error;
> > +
> > + /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> > + set_bit(EV_REL, input_dev->evbit);
> > + set_bit(EV_MSC, input_dev->evbit);
> > + /* Declare that we support "RAW" Miscellaneous events */
> > + set_bit(MSC_RAW, input_dev->mscbit);
> > +
> > + ts->input_dev = input_dev;
> > + input_set_drvdata(ts->input_dev, ts);
> > +
> > + /* Ensure that all reports are initialised to not be present. */
> > + for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> > + ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
> > +
> > + error = input_register_device(input_dev);
> > + if (error)
> > + return dev_err_probe(ts->dev, error,
> > + "Could not register with Input Sub-system.\n");
> > +
> > + error = devm_request_threaded_irq(dev, client->irq, NULL,
> > + axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
> > + if (error < 0) {
>
> if (error)
>
> > + dev_warn(dev, "Request irq failed, falling back to polling mode");
> > +
> > + error = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (error)
> > + return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
> > +
> > + if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
> > + input_set_poll_interval(input_dev, poll_interval);
> > + else
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + { "ax54a" },
> > + {},
>
> Nit: add a space inside the sentinel like you do below.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_of_match[] = {
> > + { .compatible = "touchnetix,ax54a", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > + .driver = {
> > + .name = "axiom",
> > + .of_match_table = axiom_i2c_of_match,
> > + },
> > + .id_table = axiom_i2c_id_table,
> > + .probe = axiom_i2c_probe,
> > +};
> > +module_i2c_driver(axiom_i2c_driver);
>
> Nit: please add a newline here.
>
> > +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> > +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> > +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> > +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
>
> Kind regards,
> Jeff LaBundy
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH v6 0/3] Input: Add TouchNetix axiom touchscreen driver
From: Kamel Bouhara @ 2024-01-25 16:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, Jeff LaBundy
Cc: catalin.popescu, mark.satterthwaite, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
Add a new driver for the TouchNetix's axiom family of touchscreen
controller. This driver only support i2c and can be later adapted for
SPI and USB support.
---
Changes in v2:
- Add device tree binding documentation
- Move core functions in axiom_i2c as we only care about i2c support now
- Use static function when required
- Use syntax dev_err_probe()
- Add an hardware based reset
Changes in v3:
- Remove irq-gpios property in dt-binding
- Use a generic node name
- Fix issues reported in https://lore.kernel.org/oe-kbuild-all/202310100300.oAC2M62R-lkp@intel.com/
Changes in v4:
- Cleanup unused headers and macros
- Use standard kernel type
- Namespace structures and functions
- Use packed struct when possible to avoid bitfield operators
- Fix missing break when address is found in axiom_populate_target_address()
- Split reads in two steps for the reports, first length then report
itself so we only read required bytes
- Get poll-interval from devicetree
- Add VDDI/VDDA regulators
- Add a startup delay of 110 ms required after VDDA/VDDI is applied
- Remove axiom_i2c_write() as it is no more used
Changes in v5:
- Fix wrong message constructed in axiom_i2c_read
- Delay required between i2c reads is >= 250us
- Do not split report reading in two phases as we'll
have to wait 500us
- Use lower-case in properties names
- Make regulators properties are required in dt-binding
- Fix bug report: https://lore.kernel.org/lkml/202312051457.y3N1q3sZ-lkp@intel.com/
- Fix bug report: https://lore.kernel.org/lkml/6f8e3b64-5b21-4a50-8680-063ef7a93bdb@suswa.mountain/
Changes in v6:
- Fix missing unevaluatedProperties.in dt-binding
- Use __le16 to correctly deal with device endianness
- Use standart kernel types s/char/u8/
- Use regmap api as driver might support spi later
- Use get_unaligned_le16() for the sake of clarity
- Use devm_regulator_enable_optional()
Kamel Bouhara (3):
dt-bindings: vendor-prefixes: Add TouchNetix AS
dt-bindings: input: Add TouchNetix axiom touchscreen
Input: Add TouchNetix axiom i2c touchscreen driver
.../input/touchscreen/touchnetix,ax54a.yaml | 67 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/touchnetix_axiom.c | 664 ++++++++++++++++++
6 files changed, 753 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
--
2.25.1
^ permalink raw reply
* [PATCH v6 1/3] dt-bindings: vendor-prefixes: Add TouchNetix AS
From: Kamel Bouhara @ 2024-01-25 16:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, Jeff LaBundy
Cc: catalin.popescu, mark.satterthwaite, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara,
Krzysztof Kozlowski
In-Reply-To: <20240125165823.996910-1-kamel.bouhara@bootlin.com>
Add vendor prefix for TouchNetix AS (https://www.touchnetix.com/products/).
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 573578db9509..78581001a79c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1400,6 +1400,8 @@ patternProperties:
description: Toradex AG
"^toshiba,.*":
description: Toshiba Corporation
+ "^touchnetix,.*":
+ description: TouchNetix AS
"^toumaz,.*":
description: Toumaz
"^tpk,.*":
--
2.25.1
^ permalink raw reply related
* [PATCH v6 2/3] dt-bindings: input: Add TouchNetix axiom touchscreen
From: Kamel Bouhara @ 2024-01-25 16:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, Jeff LaBundy
Cc: catalin.popescu, mark.satterthwaite, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
In-Reply-To: <20240125165823.996910-1-kamel.bouhara@bootlin.com>
Add the TouchNetix axiom I2C touchscreen device tree bindings
documentation.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
.../input/touchscreen/touchnetix,ax54a.yaml | 67 +++++++++++++++++++
MAINTAINERS | 6 ++
2 files changed, 73 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
new file mode 100644
index 000000000000..7b3997457fa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/touchnetix,ax54a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TouchNetix Axiom series touchscreen controller
+
+maintainers:
+ - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+ - $ref: /schemas/input/input.yaml#
+
+properties:
+ compatible:
+ const: touchnetix,ax54a
+
+ reg:
+ const: 0x66
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ vdda-supply:
+ description: Analog power supply regulator on VDDA pin
+
+ vddi-supply:
+ description: I/O power supply regulator on VDDI pin
+
+ startup-time-ms:
+ description: delay after power supply regulator is applied in ms
+
+required:
+ - compatible
+ - reg
+ - vdda-supply
+ - vddi-supply
+ - startup-time-ms
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@66 {
+ compatible = "touchnetix,ax54a";
+ reg = <0x66>;
+ interrupt-parent = <&gpio2>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+ vdda-supply = <&vdda_reg>;
+ vddi-supply = <&vddi_reg>;
+ poll-interval = <20>;
+ startup-time-ms = <100>;
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 7608b714653f..4752d8436dbb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21431,6 +21431,12 @@ S: Maintained
F: Documentation/ABI/testing/sysfs-class-firmware-attributes
F: drivers/platform/x86/think-lmi.?
+TOUCHNETIX AXIOM I2C TOUCHSCREEN DRIVER
+M: Kamel Bouhara <kamel.bouhara@bootlin.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
+
THUNDERBOLT DMA TRAFFIC TEST DRIVER
M: Isaac Hazan <isaac.hazan@intel.com>
L: linux-usb@vger.kernel.org
--
2.25.1
^ permalink raw reply related
* [PATCH v6 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Kamel Bouhara @ 2024-01-25 16:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, Jeff LaBundy
Cc: catalin.popescu, mark.satterthwaite, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
In-Reply-To: <20240125165823.996910-1-kamel.bouhara@bootlin.com>
Add a new driver for the TouchNetix's axiom family of
touchscreen controllers. This driver only supports i2c
and can be later adapted for SPI and USB support.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
MAINTAINERS | 1 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/touchnetix_axiom.c | 664 +++++++++++++++++++
4 files changed, 678 insertions(+)
create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4752d8436dbb..337ddac6c74b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21436,6 +21436,7 @@ M: Kamel Bouhara <kamel.bouhara@bootlin.com>
L: linux-input@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
+F: drivers/input/touchscreen/touchnetix_axiom.c
THUNDERBOLT DMA TRAFFIC TEST DRIVER
M: Isaac Hazan <isaac.hazan@intel.com>
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..f36bee8d8696 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -803,6 +803,18 @@ config TOUCHSCREEN_MIGOR
To compile this driver as a module, choose M here: the
module will be called migor_ts.
+config TOUCHSCREEN_TOUCHNETIX_AXIOM
+ tristate "TouchNetix AXIOM based touchscreen controllers"
+ depends on I2C
+ help
+ Say Y here if you have a axiom touchscreen connected to
+ your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called axiom.
+
config TOUCHSCREEN_TOUCHRIGHT
tristate "Touchright serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..8e32a2df5e18 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
+obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM) += touchnetix_axiom.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
new file mode 100644
index 000000000000..cbb5525b83f5
--- /dev/null
+++ b/drivers/input/touchscreen/touchnetix_axiom.c
@@ -0,0 +1,664 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix axiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Bart Prescott <bartp@baasheep.co.uk>
+ * Pedro Torruella <pedro.torruella@touchnetix.com>
+ * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ * Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ * Kamel Bouhara <kamel.bouhara@bootlin.com>
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+
+#include <asm/unaligned.h>
+#define AXIOM_PROX_LEVEL -128
+#define AXIOM_DMA_OPS_DELAY_USEC 250
+/*
+ * Register group u31 has 2 pages for usage table entries.
+ */
+#define AXIOM_U31_MAX_USAGES 0xff
+#define AXIOM_U31_BYTES_PER_USAGE 6
+#define AXIOM_U31_PAGE0_LENGTH 0x0C
+#define AXIOM_U31_BOOTMODE_MASK BIT(7)
+#define AXIOM_U31_DEVID_MASK GENMASK(14, 0)
+
+#define AXIOM_MAX_REPORT_LEN 0x7f
+
+#define AXIOM_CMD_HEADER_READ_MASK BIT(15)
+#define AXIOM_U41_MAX_TARGETS 10
+
+#define AXIOM_U46_AUX_CHANNELS 4
+#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
+
+#define AXIOM_COMMS_MAX_USAGE_PAGES 3
+#define AXIOM_COMMS_PAGE_SIZE 256
+#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(6, 0)
+
+#define AXIOM_REPORT_USAGE_ID 0x34
+#define AXIOM_DEVINFO_USAGE_ID 0x31
+#define AXIOM_USAGE_2HB_REPORT_ID 0x01
+#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
+#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
+
+#define AXIOM_PAGE_OFFSET_MASK GENMASK(6, 0)
+
+struct axiom_devinfo {
+ __le16 device_id;
+ u8 fw_minor;
+ u8 fw_major;
+ u8 fw_info_extra;
+ u8 tcp_revision;
+ u8 bootloader_fw_minor;
+ u8 bootloader_fw_major;
+ __le16 jedec_id;
+ u8 num_usages;
+} __packed;
+
+/*
+ * Describes parameters of a specific usage, essentially a single element of
+ * the "Usage Table"
+ */
+struct axiom_usage_entry {
+ u8 id;
+ u8 is_report;
+ u8 start_page;
+ u8 num_pages;
+};
+
+/*
+ * Represents state of a touch or target when detected prior to a touch (eg.
+ * hover or proximity events).
+ */
+enum axiom_target_state {
+ AXIOM_TARGET_STATE_NOT_PRESENT = 0,
+ AXIOM_TARGET_STATE_PROX = 1,
+ AXIOM_TARGET_STATE_HOVER = 2,
+ AXIOM_TARGET_STATE_TOUCHING = 3,
+};
+
+struct axiom_u41_target {
+ enum axiom_target_state state;
+ u16 x;
+ u16 y;
+ s8 z;
+ bool insert;
+ bool touch;
+};
+
+struct axiom_target_report {
+ u8 index;
+ u8 present;
+ u16 x;
+ u16 y;
+ s8 z;
+};
+
+struct axiom_cmd_header {
+ __le16 target_address;
+ __le16 length;
+} __packed;
+
+struct axiom_data {
+ struct axiom_devinfo devinfo;
+ struct device *dev;
+ struct gpio_desc *reset_gpio;
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ u32 max_report_len;
+ u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
+ struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
+ struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
+ bool usage_table_populated;
+ struct regulator *vdda;
+ struct regulator *vddi;
+ struct regmap *regmap;
+ struct touchscreen_properties prop;
+};
+
+static const struct regmap_config axiom_i2c_regmap_config = {
+ .reg_bits = 32,
+ .reg_format_endian = REGMAP_ENDIAN_LITTLE,
+ .val_bits = 8,
+ .val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+/*
+ * axiom devices are typically configured to report touches at a rate
+ * of 100Hz (10ms) for systems that require polling for reports.
+ * When reports are polled, it will be expected to occasionally
+ * observe the overflow bit being set in the reports.
+ * This indicates that reports are not being read fast enough.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 10
+
+/* Translate usage/page/offset triplet into physical address. */
+static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8 usage, u8 page,
+ char offset)
+{
+ /* At the moment the convention is that u31 is always at physical address 0x0 */
+ if (!ts->usage_table_populated) {
+ if (usage == AXIOM_DEVINFO_USAGE_ID)
+ return ((page << 8) + offset);
+ else
+ return 0xffff;
+ }
+
+ if (page >= ts->usage_table[usage].num_pages) {
+ dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x, offset: %02x\n",
+ usage, page, offset);
+ return 0xffff;
+ }
+
+ return ((ts->usage_table[usage].start_page + page) << 8) + offset;
+}
+
+static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void *buf, u16 len)
+{
+ struct axiom_cmd_header cmd_header;
+ int ret;
+
+ cmd_header.target_address = cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
+ cmd_header.length = cpu_to_le16(len | AXIOM_CMD_HEADER_READ_MASK);
+
+ __le32 preamble = get_unaligned_le32((u8 *)&cmd_header);
+
+ ret = regmap_write(ts->regmap, preamble, 0);
+ if (ret) {
+ dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
+ return ret;
+ }
+
+ ret = regmap_raw_read(ts->regmap, 0, buf, len);
+ if (ret) {
+ dev_err(ts->dev, "failed to read target address %04x, error %d\n",
+ cmd_header.target_address, ret);
+ return ret;
+ }
+
+ /* Wait device's DMA operations */
+ usleep_range(AXIOM_DMA_OPS_DELAY_USEC, AXIOM_DMA_OPS_DELAY_USEC + 50);
+
+ return 0;
+}
+
+/*
+ * One of the main purposes for reading the usage table is to identify
+ * which usages reside at which target address.
+ * When performing subsequent reads or writes to AXIOM, the target address
+ * is used to specify which usage is being accessed.
+ * Consider the following discovery code which will build up the usage table.
+ */
+static u32 axiom_populate_usage_table(struct axiom_data *ts)
+{
+ struct axiom_usage_entry *usage_table;
+ u8 *rx_data = ts->rx_buf;
+ u32 max_report_len;
+ u32 usage_id;
+ int error;
+
+ usage_table = ts->usage_table;
+
+ /* Read the second page of usage u31 to get the usage table */
+ error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
+ (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+
+ if (error)
+ return error;
+
+ for (usage_id = 1; usage_id < AXIOM_U31_MAX_USAGES; usage_id++) {
+ u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
+ u8 id = rx_data[offset + 0];
+ u8 start_page = rx_data[offset + 1];
+ u8 num_pages = rx_data[offset + 2];
+ u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
+
+ if (!num_pages)
+ usage_table[id].is_report = true;
+
+ /* Store the entry into the usage table */
+ usage_table[id].id = id;
+ usage_table[id].start_page = start_page;
+ usage_table[id].num_pages = num_pages;
+
+ dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id, AXIOM_U31_BYTES_PER_USAGE,
+ &rx_data[offset]);
+
+ /* Identify the max report length the module will receive */
+ if (usage_table[id].is_report && max_offset > max_report_len)
+ max_report_len = max_offset;
+ }
+
+ ts->usage_table_populated = true;
+
+ return max_report_len;
+}
+
+static int axiom_discover(struct axiom_data *ts)
+{
+ int error;
+
+ /*
+ * Fetch the first page of usage u31 to get the
+ * device information and the number of usages
+ */
+ error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo, AXIOM_U31_PAGE0_LENGTH);
+ if (error)
+ return error;
+
+ dev_dbg(ts->dev, " Boot Mode : %s\n",
+ FIELD_GET(AXIOM_U31_BOOTMODE_MASK,
+ le16_to_cpu(ts->devinfo.device_id)) ? "BLP" : "TCP");
+ dev_dbg(ts->dev, " Device ID : %04lx\n",
+ FIELD_GET(AXIOM_U31_DEVID_MASK, le16_to_cpu(ts->devinfo.device_id)));
+ dev_dbg(ts->dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
+ ts->devinfo.fw_minor);
+ dev_dbg(ts->dev, " Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
+ ts->devinfo.bootloader_fw_minor);
+ dev_dbg(ts->dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
+ dev_dbg(ts->dev, " Silicon : %04x\n", le16_to_cpu(ts->devinfo.jedec_id));
+ dev_dbg(ts->dev, " Number usages : %04x\n", ts->devinfo.num_usages);
+
+ ts->max_report_len = axiom_populate_usage_table(ts);
+ if (!ts->max_report_len || !ts->devinfo.num_usages ||
+ ts->max_report_len > AXIOM_MAX_REPORT_LEN) {
+ dev_err(ts->dev, "Invalid report length or usages number");
+ return -EINVAL;
+ }
+
+ dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
+
+ return 0;
+}
+
+/*
+ * Support function to axiom_process_u41_report.
+ * Generates input-subsystem events for every target.
+ * After calling this function the caller shall issue
+ * a Sync to the input sub-system.
+ */
+static bool axiom_process_u41_report_target(struct axiom_data *ts,
+ struct axiom_target_report *target)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ struct axiom_u41_target *target_prev_state;
+ enum axiom_target_state current_state;
+ int slot;
+
+ /* Verify the target index */
+ if (target->index >= AXIOM_U41_MAX_TARGETS) {
+ dev_err(ts->dev, "Invalid target index! %u\n", target->index);
+ return false;
+ }
+
+ target_prev_state = &ts->targets[target->index];
+
+ current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
+
+ if (target->present) {
+ if (target->z >= 0)
+ current_state = AXIOM_TARGET_STATE_TOUCHING;
+ else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
+ current_state = AXIOM_TARGET_STATE_HOVER;
+ else if (target->z == AXIOM_PROX_LEVEL)
+ current_state = AXIOM_TARGET_STATE_PROX;
+ }
+
+ if (target_prev_state->state == current_state &&
+ target_prev_state->x == target->x &&
+ target_prev_state->y == target->y &&
+ target_prev_state->z == target->z)
+ return false;
+
+ slot = target->index;
+
+ dev_dbg(ts->dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
+ target->index, slot, target->present,
+ target->x, target->y, target->z);
+
+ switch (current_state) {
+ case AXIOM_TARGET_STATE_NOT_PRESENT:
+ case AXIOM_TARGET_STATE_PROX:
+ if (!target_prev_state->insert)
+ break;
+ target_prev_state->insert = false;
+ input_mt_slot(input_dev, slot);
+
+ if (!slot)
+ input_report_key(input_dev, BTN_TOUCH, 0);
+
+ input_mt_report_slot_inactive(input_dev);
+ /*
+ * make sure the previous coordinates are
+ * all off screen when the finger comes back
+ */
+ target->x = 65535;
+ target->y = 65535;
+ target->z = AXIOM_PROX_LEVEL;
+ break;
+ case AXIOM_TARGET_STATE_HOVER:
+ case AXIOM_TARGET_STATE_TOUCHING:
+ target_prev_state->insert = true;
+ input_mt_slot(input_dev, slot);
+ input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
+ input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
+ input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
+
+ if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
+ input_report_abs(input_dev, ABS_PRESSURE, target->z);
+ } else {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
+ input_report_abs(input_dev, ABS_PRESSURE, 0);
+ }
+
+ if (!slot)
+ input_report_key(input_dev, BTN_TOUCH, (current_state ==
+ AXIOM_TARGET_STATE_TOUCHING));
+ break;
+ default:
+ break;
+ }
+
+ target_prev_state->state = current_state;
+ target_prev_state->x = target->x;
+ target_prev_state->y = target->y;
+ target_prev_state->z = target->z;
+
+ return true;
+}
+
+/*
+ * U41 is the output report of the 2D CTS and contains the status of targets
+ * (including contacts and pre-contacts) along with their X,Y,Z values.
+ * When a target has been removed (no longer detected),
+ * the corresponding X,Y,Z values will be zeroed.
+ */
+static bool axiom_process_u41_report(struct axiom_data *ts, u8 *rx_buf)
+{
+ struct axiom_target_report target;
+ bool update_done = false;
+ u16 target_status;
+ int i;
+
+ target_status = get_unaligned_le16(rx_buf + 1);
+
+ for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
+ u8 *target_step = &rx_buf[i * 4];
+
+ target.index = i;
+ target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
+ target.x = get_unaligned_le16(target_step + 3);
+ target.y = get_unaligned_le16(target_step + 5);
+ target.z = (s8)(rx_buf[i + 43]);
+ update_done |= axiom_process_u41_report_target(ts, &target);
+ }
+
+ return update_done;
+}
+
+/*
+ * U46 report contains a low level measurement data generated by the capacitive
+ * displacement sensor (CDS) algorithms from the auxiliary channels.
+ * This information is useful when tuning multi-press to assess mechanical
+ * consistency in the unit's construction.
+ */
+static void axiom_process_u46_report(struct axiom_data *ts, u8 *rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ u32 event_value;
+ u16 aux_value;
+ int i;
+
+ for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
+ u8 *target_step = &rx_buf[i * 2];
+
+ aux_value = get_unaligned_le16(target_step + 1) & AXIOM_U46_AUX_MASK;
+ event_value = (i << 16) | (aux_value);
+ input_event(input_dev, EV_MSC, MSC_RAW, event_value);
+ }
+}
+
+/*
+ * Validates the crc and demultiplexes the axiom reports to the appropriate
+ * report handler
+ */
+static int axiom_handle_events(struct axiom_data *ts)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ u8 *report_data = ts->rx_buf;
+ struct device *dev = ts->dev;
+ u16 crc_report;
+ u16 crc_calc;
+ int error;
+ u8 len;
+
+ error = axiom_read(ts, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
+ if (error)
+ return error;
+
+ len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
+ if (len <= 2) {
+ dev_err(dev, "Zero length report discarded.\n");
+ return -ENODATA;
+ }
+
+ /* Validate the report CRC */
+ u8 *crc_bytes = &report_data[len];
+
+ crc_report = get_unaligned_le16(crc_bytes - 2);
+ /* Length is in 16 bit words and remove the size of the CRC16 itself */
+ crc_calc = crc16(0, report_data, (len - 2));
+
+ if (crc_calc != crc_report) {
+ dev_err(dev,
+ "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
+ crc_report, crc_calc);
+ return -EINVAL;
+ }
+
+ switch (report_data[1]) {
+ case AXIOM_USAGE_2DCTS_REPORT_ID:
+ if (axiom_process_u41_report(ts, &report_data[1])) {
+ input_mt_sync_frame(input_dev);
+ input_sync(input_dev);
+ }
+ break;
+
+ case AXIOM_USAGE_2AUX_REPORT_ID:
+ /* This is an aux report (force) */
+ axiom_process_u46_report(ts, &report_data[1]);
+ input_mt_sync(input_dev);
+ input_sync(input_dev);
+ break;
+
+ case AXIOM_USAGE_2HB_REPORT_ID:
+ /* This is a heartbeat report */
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void axiom_i2c_poll(struct input_dev *input_dev)
+{
+ struct axiom_data *ts = input_get_drvdata(input_dev);
+
+ axiom_handle_events(ts);
+}
+
+static irqreturn_t axiom_irq(int irq, void *dev_id)
+{
+ struct axiom_data *ts = dev_id;
+
+ axiom_handle_events(ts);
+
+ return IRQ_HANDLED;
+}
+
+static void axiom_reset(struct gpio_desc *reset_gpio)
+{
+ gpiod_set_value_cansleep(reset_gpio, 1);
+ usleep_range(1000, 2000);
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ msleep(110);
+}
+
+static int axiom_i2c_probe(struct i2c_client *client)
+{
+ u32 poll_interval = POLL_INTERVAL_DEFAULT_MS;
+ struct device *dev = &client->dev;
+ struct input_dev *input_dev;
+ struct axiom_data *ts;
+ u32 startup_delay_ms;
+ int target;
+ int error;
+
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, ts);
+ ts->client = client;
+ ts->dev = dev;
+
+ ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
+ error = PTR_ERR_OR_ZERO(ts->regmap);
+ if (error) {
+ dev_err(dev, "Failed to initialize regmap: %d\n", error);
+ return error;
+ }
+
+ ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
+
+ if (ts->reset_gpio)
+ axiom_reset(ts->reset_gpio);
+
+ ts->vddi = devm_regulator_get_optional(dev, "vddi");
+ if (!IS_ERR(ts->vddi)) {
+ error = devm_regulator_get_enable(dev, "vddi");
+ if (error)
+ return dev_err_probe(&client->dev, error,
+ "Failed to enable vddi regulator\n");
+ }
+
+ ts->vdda = devm_regulator_get_optional(dev, "vdda");
+ if (!IS_ERR(ts->vdda)) {
+ error = devm_regulator_get_enable(dev, "vdda");
+ if (error)
+ return dev_err_probe(&client->dev, error,
+ "Failed to enable vdda regulator\n");
+ if (!device_property_read_u32(dev, "startup-time-ms", &startup_delay_ms))
+ msleep(startup_delay_ms);
+ }
+
+ error = axiom_discover(ts);
+ if (error)
+ return dev_err_probe(dev, error, "Failed touchscreen discover\n");
+
+ input_dev = devm_input_allocate_device(ts->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ input_dev->name = "TouchNetix axiom Touchscreen";
+ input_dev->phys = "input/axiom_ts";
+
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
+
+ touchscreen_parse_properties(input_dev, true, &ts->prop);
+
+ /* Registers the axiom device as a touchscreen instead of a mouse pointer */
+ error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
+ if (error)
+ return error;
+
+ /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
+ set_bit(EV_REL, input_dev->evbit);
+ set_bit(EV_MSC, input_dev->evbit);
+ /* Declare that we support "RAW" Miscellaneous events */
+ set_bit(MSC_RAW, input_dev->mscbit);
+
+ ts->input_dev = input_dev;
+ input_set_drvdata(ts->input_dev, ts);
+
+ /* Ensure that all reports are initialised to not be present. */
+ for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
+ ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
+
+ error = input_register_device(input_dev);
+ if (error)
+ return dev_err_probe(ts->dev, error,
+ "Could not register with Input Sub-system.\n");
+
+ error = devm_request_threaded_irq(dev, client->irq, NULL,
+ axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
+ if (error) {
+ dev_info(dev, "Request irq failed, falling back to polling mode");
+
+ error = input_setup_polling(input_dev, axiom_i2c_poll);
+ if (error)
+ return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
+
+ if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
+ input_set_poll_interval(input_dev, poll_interval);
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+ { "ax54a" },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_of_match[] = {
+ { .compatible = "touchnetix,ax54a", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
+
+static struct i2c_driver axiom_i2c_driver = {
+ .driver = {
+ .name = "axiom",
+ .of_match_table = axiom_i2c_of_match,
+ },
+ .id_table = axiom_i2c_id_table,
+ .probe = axiom_i2c_probe,
+};
+module_i2c_driver(axiom_i2c_driver);
+
+MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
+MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
+MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
+MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related
* Re: [HID Patchsets for Samsung driver v4 2/6] HID: Samsung : Fix the checkpatch complain. Rewritten code using memcmp where applicable.
From: Joe Perches @ 2024-01-25 21:54 UTC (permalink / raw)
To: Sandeep C S, Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-3-sandeep.cs@samsung.com>
On Thu, 2024-01-25 at 10:06 +0530, Sandeep C S wrote:
> Resolved warnings found by checkpatch.pl script.
[]
> diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
> @@ -107,17 +99,39 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
>
> switch (usage->hid & HID_USAGE) {
> /* report 2 */
> - case 0x183: samsung_kbd_mouse_map_key_clear(KEY_MEDIA); break;
> - case 0x195: samsung_kbd_mouse_map_key_clear(KEY_EMAIL); break;
> - case 0x196: samsung_kbd_mouse_map_key_clear(KEY_CALC); break;
> - case 0x197: samsung_kbd_mouse_map_key_clear(KEY_COMPUTER); break;
> - case 0x22b: samsung_kbd_mouse_map_key_clear(KEY_SEARCH); break;
> - case 0x22c: samsung_kbd_mouse_map_key_clear(KEY_WWW); break;
> - case 0x22d: samsung_kbd_mouse_map_key_clear(KEY_BACK); break;
> - case 0x22e: samsung_kbd_mouse_map_key_clear(KEY_FORWARD); break;
> - case 0x22f: samsung_kbd_mouse_map_key_clear(KEY_FAVORITES); break;
> - case 0x230: samsung_kbd_mouse_map_key_clear(KEY_REFRESH); break;
> - case 0x231: samsung_kbd_mouse_map_key_clear(KEY_STOP); break;
It's rather smaller code to remove the call duplication and use
a static const struct and for loop as suggested here:
https://lore.kernel.org/lkml/10aeef4ddd523b85ab34327bf384119d0d4b6567.camel@perches.com/
^ permalink raw reply
* Re: [PATCH v6 7/7] x86/vmware: Add TDX hypercall support
From: Alexey Makhalov @ 2024-01-26 0:55 UTC (permalink / raw)
To: H. Peter Anvin, Dave Hansen, linux-kernel, virtualization, bp,
dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, namit, timothym, akaher,
jsipek, dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <351B1153-9CBE-4774-9FAF-770F9F36856E@zytor.com>
On 1/22/24 4:17 PM, H. Peter Anvin wrote:
> On January 22, 2024 4:04:33 PM PST, Alexey Makhalov <alexey.makhalov@broadcom.com> wrote:
>>
>>
>> On 1/22/24 10:28 AM, H. Peter Anvin wrote:
>>> On January 22, 2024 8:32:22 AM PST, Dave Hansen <dave.hansen@intel.com> wrote:
>>>> On 1/9/24 00:40, Alexey Makhalov wrote:
>>>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>>>> +unsigned long vmware_tdx_hypercall(unsigned long cmd,
>>>>> + struct tdx_module_args *args)
>>>>> +{
>>>>> + if (!hypervisor_is_type(X86_HYPER_VMWARE))
>>>>> + return ULONG_MAX;
>>>>> +
>>>>> + if (cmd & ~VMWARE_CMD_MASK) {
>>>>> + pr_warn_once("Out of range command %lx\n", cmd);
>>>>> + return ULONG_MAX;
>>>>> + }
>>>>> +
>>>>> + args->r10 = VMWARE_TDX_VENDOR_LEAF;
>>>>> + args->r11 = VMWARE_TDX_HCALL_FUNC;
>>>>> + args->r12 = VMWARE_HYPERVISOR_MAGIC;
>>>>> + args->r13 = cmd;
>>>>> + args->r15 = 0; /* CPL */
>>>>> +
>>>>> + __tdx_hypercall(args);
>>>>> +
>>>>> + return args->r12;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
>>>>> +#endif
>>>>
>>>> This is the kind of wrapper that I was hoping for. Thanks.
>>>>
>>>> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>>>>
>>>
>>> I'm slightly confused by this TBH.
>>>
>>> Why are the arguments passed in as a structure, which is modified by the wrapper to boot? This is analogous to a system call interface.
>>>
>>> Furthermore, this is an out-of-line function; it should never be called with !X86_HYPER_VMWARE or you are introducing overhead for other hypervisors; I believe a pr_warn_once() is in order at least, just as you have for the out-of-range test.
>>>
>>
>> This patch series introduces vmware_hypercall family of functions similar to kvm_hypercall. Similarity: both vmware and kvm implementations are static inline functions and both of them use __tdx_hypercall (global not exported symbol). Difference: kvm_hypercall functions are used _only_ within the kernel, but vmware_hypercall are also used by modules.
>> Exporting __tdx_hypercall function is an original Dave's concern.
>> So we ended up with exporting wrapper, not generic, but VMware specific with added checks against arbitrary use.
>> vmware_tdx_hypercall is not designed for !X86_HYPER_VMWARE callers. But such a calls are not forbidden.
>> Arguments in a structure is an API for __tdx_hypercall(). Input and output argument handling are done by vmware_hypercall callers, while VMware specific dress up is inside the wrapper.
>>
>> Peter, do you think code comments are required to make it clear for the reader?
>>
>>
>
> TBH that explanation didn't make much sense to me...
Peter,
I would like to understand your concerns.
1. Are you suggesting to move structure (tdx parameters) initialization
in one please, instead of one part there another part here? Do you
prefer to pass all arguments as is to vmware_tdx_hypercall() and only
define tdx_module_args there?
2. And second suggestion is to add pr_warn_once under "if
(!hypervisor_is_type(X86_HYPER_VMWARE))" ?
--Alexey
^ permalink raw reply
* [PATCH 0/2] arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen
From: Bjorn Andersson @ 2024-01-26 3:55 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson
This documents and defines the necessary properties for the I2C
HID-based touchscreen found in some SKUs of the Lenovo Thinkpad X13s to
work.
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Bjorn Andersson (2):
dt-bindings: HID: i2c-hid: Document reset-related properties
arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++++-
2 files changed, 11 insertions(+), 1 deletion(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e
Best regards,
--
Bjorn Andersson <quic_bjorande@quicinc.com>
^ permalink raw reply
* [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-26 3:55 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson
In-Reply-To: <20240125-x13s-touchscreen-v1-0-ab8c882def9c@quicinc.com>
The failing read-test in __i2c_hid_core_probe() determines that there's
nothing connected at the documented address of the touchscreen.
Introduce the 5ms after-power and 200ms after-reset delays found in the
ACPI tables. Also wire up the reset-gpio, for good measure.
Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index def3976bd5bb..d64d0e76c1ea 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -620,7 +620,6 @@ &i2c4 {
status = "okay";
- /* FIXME: verify */
touchscreen@10 {
compatible = "hid-over-i2c";
reg = <0x10>;
@@ -630,6 +629,11 @@ touchscreen@10 {
vdd-supply = <&vreg_misc_3p3>;
vddl-supply = <&vreg_s10b>;
+ reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+ post-power-on-delay-ms = <5>;
+ post-reset-deassert-delay-ms = <200>;
+
pinctrl-names = "default";
pinctrl-0 = <&ts0_default>;
};
--
2.25.1
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties
From: Bjorn Andersson @ 2024-01-26 3:55 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson
In-Reply-To: <20240125-x13s-touchscreen-v1-0-ab8c882def9c@quicinc.com>
Some I2C HID devices has a reset pin and requires that some specified
time elapses after this reset pin is deasserted, before communication
with the device is attempted.
The Linux implementation is looking for these in the "reset-gpios" and
"post-reset-deassert-delay-ms" properties already, so use these property
names.
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 138caad96a29..f07ff4cb3d26 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -50,6 +50,12 @@ properties:
description: Time required by the device after enabling its regulators
or powering it on, before it is ready for communication.
+ post-reset-deassert-delay-ms:
+ description: Time required by the device after reset has been deasserted,
+ before it is ready for communication.
+
+ reset-gpios: true
+
touchscreen-inverted-x: true
touchscreen-inverted-y: true
--
2.25.1
^ permalink raw reply related
* [PATCH] Input: iqs7222 - add support for IQS7222D v1.1 and v1.2
From: Jeff LaBundy @ 2024-01-26 4:02 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, jeff
The vendor has introduced two new revisions with slightly different
memory maps; update the driver to support them.
Fixes: dd24e202ac72 ("Input: iqs7222 - add support for Azoteq IQS7222D")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
drivers/input/misc/iqs7222.c | 112 +++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index 36aeeae77611..9ca5a743f19f 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -620,6 +620,118 @@ static const struct iqs7222_dev_desc iqs7222_devs[] = {
},
},
},
+ {
+ .prod_num = IQS7222_PROD_NUM_D,
+ .fw_major = 1,
+ .fw_minor = 2,
+ .touch_link = 1770,
+ .allow_offset = 9,
+ .event_offset = 10,
+ .comms_offset = 11,
+ .reg_grps = {
+ [IQS7222_REG_GRP_STAT] = {
+ .base = IQS7222_SYS_STATUS,
+ .num_row = 1,
+ .num_col = 7,
+ },
+ [IQS7222_REG_GRP_CYCLE] = {
+ .base = 0x8000,
+ .num_row = 7,
+ .num_col = 2,
+ },
+ [IQS7222_REG_GRP_GLBL] = {
+ .base = 0x8700,
+ .num_row = 1,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_BTN] = {
+ .base = 0x9000,
+ .num_row = 14,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_CHAN] = {
+ .base = 0xA000,
+ .num_row = 14,
+ .num_col = 4,
+ },
+ [IQS7222_REG_GRP_FILT] = {
+ .base = 0xAE00,
+ .num_row = 1,
+ .num_col = 2,
+ },
+ [IQS7222_REG_GRP_TPAD] = {
+ .base = 0xB000,
+ .num_row = 1,
+ .num_col = 24,
+ },
+ [IQS7222_REG_GRP_GPIO] = {
+ .base = 0xC000,
+ .num_row = 3,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_SYS] = {
+ .base = IQS7222_SYS_SETUP,
+ .num_row = 1,
+ .num_col = 12,
+ },
+ },
+ },
+ {
+ .prod_num = IQS7222_PROD_NUM_D,
+ .fw_major = 1,
+ .fw_minor = 1,
+ .touch_link = 1774,
+ .allow_offset = 9,
+ .event_offset = 10,
+ .comms_offset = 11,
+ .reg_grps = {
+ [IQS7222_REG_GRP_STAT] = {
+ .base = IQS7222_SYS_STATUS,
+ .num_row = 1,
+ .num_col = 7,
+ },
+ [IQS7222_REG_GRP_CYCLE] = {
+ .base = 0x8000,
+ .num_row = 7,
+ .num_col = 2,
+ },
+ [IQS7222_REG_GRP_GLBL] = {
+ .base = 0x8700,
+ .num_row = 1,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_BTN] = {
+ .base = 0x9000,
+ .num_row = 14,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_CHAN] = {
+ .base = 0xA000,
+ .num_row = 14,
+ .num_col = 4,
+ },
+ [IQS7222_REG_GRP_FILT] = {
+ .base = 0xAE00,
+ .num_row = 1,
+ .num_col = 2,
+ },
+ [IQS7222_REG_GRP_TPAD] = {
+ .base = 0xB000,
+ .num_row = 1,
+ .num_col = 24,
+ },
+ [IQS7222_REG_GRP_GPIO] = {
+ .base = 0xC000,
+ .num_row = 3,
+ .num_col = 3,
+ },
+ [IQS7222_REG_GRP_SYS] = {
+ .base = IQS7222_SYS_SETUP,
+ .num_row = 1,
+ .num_col = 12,
+ },
+ },
+ },
{
.prod_num = IQS7222_PROD_NUM_D,
.fw_major = 0,
--
2.34.1
^ permalink raw reply related
* Re: PS/2 keyboard of laptop Dell XPS 13 9360 goes missing after S3
From: Paul Menzel @ 2024-01-26 7:03 UTC (permalink / raw)
To: Hans de Goede
Cc: linux-input, linux-pm, Dell.Client.Kernel, regressions,
linux-kernel
In-Reply-To: <dde1bdfe-7877-41bd-b233-03bcdba0e2de@redhat.com>
Dear Hans,
Thank you for your reply, and sorry for the delay on my side. I needed
to set up an environment to easily build the Linux kernel.
Am 22.01.24 um 14:43 schrieb Hans de Goede:
> On 1/21/24 15:26, Paul Menzel wrote:
[…]
>> Am 20.01.24 um 21:26 schrieb Hans de Goede:
>>
>>> On 1/18/24 13:57, Paul Menzel wrote:
>>>> #regzbot introduced v6.6.11..v6.7
>>
>>>> There seems to be a regression in Linux 6.7 on the Dell XPS 13 9360 (Intel i7-7500U).
>>>>
>>>> [ 0.000000] DMI: Dell Inc. XPS 13 9360/0596KF, BIOS 2.21.0 06/02/2022
>>>>
>>>> The PS/2 keyboard goes missing after S3 resume¹. The problem does not happen with Linux 6.6.11.
>>>
>>> Thank you for reporting this.
>>>
>>> Can you try adding "i8042.dumbkbd=1" to your kernel commandline?
>>>
>>> This should at least lead to the device not disappearing from
>>>
>>> "sudo libinput list-devices"
>>>
>>> The next question is if the keyboard will still actually
>>> work after suspend/resume with "i8042.dumbkbd=1". If it
>>> stays in the list, but no longer works then there is
>>> a problem with the i8042 controller; or interrupt
>>> delivery to the i8042 controller.
>>>
>>> If "i8042.dumbkbd=1" somehow fully fixes things, then I guess
>>> my atkbd driver fix for other laptop keyboards is somehow
>>> causing issues for yours.
>>
>> Just a quick feedback, that booting with `i8042.dumbkbd=1` seems to fix the issue.
>>
>>> If "i8042.dumbkbd=1" fully fixes things, can you try building
>>> your own 6.7.0 kernel with commit 936e4d49ecbc:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=936e4d49ecbc8c404790504386e1422b599dec39
>>>
>>> reverted?
>>
>> I am going to try that as soon as possible.
>
> Assuming this was not some one time glitch with 6.7.0,
> I have prepared a patch hopefully fixing this (1) as well
> as a follow up fix to address another potential issue which
> I have noticed.
Unfortunately, it wasn’t just a glitch.
> Can you please give a 6.7.0 (2) kernel with the 2 attached
> patches added a try ?
>
> I know building kernels can be a bit of work / takes time,
> sorry. If you are short on time I would prefer testing these 2
> patches and see if they fix things over trying a plain revert.
Applying both patches on v6.7.1
$ git log --oneline -3
053fa44c0de1 (HEAD -> v6.7.1) Input: atkbd - Do not skip
atkbd_deactivate() when skipping ATKBD_CMD_GETID
0e0fa0113c7a Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping
ATKBD_CMD_GETID
a91fdae50a6d (tag: v6.7.1, stable/linux-6.7.y, origin/linux-6.7.y)
Linux 6.7.1
I am unable to reproduce the problem in eight ACPI S3 suspend/resume
cycles. The DMAR errors [3] are also gone:
$ sudo dmesg --level alert,crit,err,warn
[ 0.065292] MDS CPU bug present and SMT on, data leak possible.
See https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html
for more details.
[ 0.065292] MMIO Stale Data CPU bug present and SMT on, data
leak possible. See
https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/processor_mmio_stale_data.html
for more details.
[ 0.092064] ENERGY_PERF_BIAS: Set to 'normal', was 'performance'
[ 0.294522] hpet_acpi_add: no address or irqs in _CRS
[ 0.345003] i8042: Warning: Keylock active
[ 1.063807] usb: port power management may be unreliable
[ 1.178339] device-mapper: core: CONFIG_IMA_DISABLE_HTABLE is
disabled. Duplicate IMA measurements will not be recorded in the IMA log.
[ 37.712916] wmi_bus wmi_bus-PNP0C14:01: WQBC data block query
control method not found
[ 67.307070] warning: `atop' uses wireless extensions which will
stop working for Wi-Fi 7 hardware; use nl80211
[ 141.861803] ACPI Error: AE_BAD_PARAMETER, Returned by Handler
for [EmbeddedControl] (20230628/evregion-300)
[ 141.861808] ACPI Error: Aborting method \_SB.PCI0.LPCB.ECDV.ECR1
due to previous error (AE_BAD_PARAMETER) (20230628/psparse-529)
[ 141.861814] ACPI Error: Aborting method \_SB.PCI0.LPCB.ECDV.ECR2
due to previous error (AE_BAD_PARAMETER) (20230628/psparse-529)
[ 141.861818] ACPI Error: Aborting method \ECRW due to previous
error (AE_BAD_PARAMETER) (20230628/psparse-529)
[ 141.861821] ACPI Error: Aborting method \ECG1 due to previous
error (AE_BAD_PARAMETER) (20230628/psparse-529)
[ 141.861824] ACPI Error: Aborting method \NEVT due to previous
error (AE_BAD_PARAMETER) (20230628/psparse-529)
[ 141.861827] ACPI Error: Aborting method \_SB.PCI0.LPCB.ECDV._Q66
due to previous error (AE_BAD_PARAMETER) (20230628/psparse-529)
Please tell me, if I can do anything else.
Kind regards,
Paul
> 1) Assuming it is caused by this commit in the first place,
> which seems likely
>
> 2) 6.8-rc1 has a follow up patch which is squashed into the
> first patch here, so these patches will only apply cleanly
> to 6.7.0 .
[3]:
https://lore.kernel.org/all/9a24c335-8ec5-48c9-9bdd-b0dac5ecbca8@molgen.mpg.de/
>>>> [ 1.435071] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M] at 0x60,0x64 irq 1,12
>>>> [ 1.435409] i8042: Warning: Keylock active
>>>> [ 1.437624] serio: i8042 KBD port at 0x60,0x64 irq 1
>>>> [ 1.437631] serio: i8042 AUX port at 0x60,0x64 irq 12
>>>> […]
>>>> [ 1.439743] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
>>>>
>>>> $ sudo libinput list-devices
>>>> […]
>>>> Device: AT Translated Set 2 keyboard
>>>> Kernel: /dev/input/event0
>>>> Group: 15
>>>> Seat: seat0, default
>>>> Capabilities: keyboard
>>>> Tap-to-click: n/a
>>>> Tap-and-drag: n/a
>>>> Tap drag lock: n/a
>>>> Left-handed: n/a
>>>> Nat.scrolling: n/a
>>>> Middle emulation: n/a
>>>> Calibration: n/a
>>>> Scroll methods: none
>>>> Click methods: none
>>>> Disable-w-typing: n/a
>>>> Disable-w-trackpointing: n/a
>>>> Accel profiles: n/a
>>>> Rotation: 0.0
>>>>
>>>> `libinput list-devices` does not list the device after resuming
>>>> from S3. Some of the function keys, like brightness and airplane
>>>> mode keys, still work, as the events are probably transmitted over
>>>> the embedded controller or some other mechanism. An external USB
>>>> keyboard also still works.
>>>>
>>>> I haven’t had time to further analyze this, but wanted to report
>>>> it. No idea
>>>>
>>>>
>>>> Kind regards,
>>>>
>>>> Paul
>>>>
>>>>
>>>> ¹ s2idle is not working correctly on the device, in the sense, that
>>>> energy usage is very high in that state, and the full battery is at
>>>> 20 % after leaving it for eight hours.
^ permalink raw reply
* [PATCH] HID: nintendo: Remove some unused functions
From: Jiapeng Chong @ 2024-01-26 7:54 UTC (permalink / raw)
To: djogorchock
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel,
Jiapeng Chong, Abaci Robot
These functions are defined in the hid-nintendo.c file, but not called
elsewhere, so delete these unused functions.
drivers/hid/hid-nintendo.c:757:20: warning: unused function 'joycon_type_has_left_controls'.
drivers/hid/hid-nintendo.c:763:20: warning: unused function 'joycon_type_has_right_controls'.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8060
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
drivers/hid/hid-nintendo.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 7ce6be0a8dee..ab5953fc2436 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -754,18 +754,6 @@ static inline bool joycon_type_is_right_nescon(struct joycon_ctlr *ctlr)
return ctlr->ctlr_type == JOYCON_CTLR_TYPE_NESR;
}
-static inline bool joycon_type_has_left_controls(struct joycon_ctlr *ctlr)
-{
- return joycon_type_is_left_joycon(ctlr) ||
- joycon_type_is_procon(ctlr);
-}
-
-static inline bool joycon_type_has_right_controls(struct joycon_ctlr *ctlr)
-{
- return joycon_type_is_right_joycon(ctlr) ||
- joycon_type_is_procon(ctlr);
-}
-
static inline bool joycon_type_is_any_joycon(struct joycon_ctlr *ctlr)
{
return joycon_type_is_left_joycon(ctlr) ||
--
2.20.1.7.g153144c
^ permalink raw reply related
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Johan Hovold @ 2024-01-26 8:12 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240125-x13s-touchscreen-v1-2-ab8c882def9c@quicinc.com>
On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> The failing read-test in __i2c_hid_core_probe() determines that there's
> nothing connected at the documented address of the touchscreen.
>
> Introduce the 5ms after-power and 200ms after-reset delays found in the
> ACPI tables. Also wire up the reset-gpio, for good measure.
As the supplies for the touchscreen are always on (and left on by the
bootloader) it would seem that it is really the addition of the reset
gpio which makes things work here. Unless the delay is needed for some
other reason.
(The power-on delay also looks a bit short compared to what is used for
other devices.)
Reset support was only recently added with commit 2be404486c05 ("HID:
i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
backport this one before first determining that.
That commit also added a comment in the HID driver about the
'post-reset-deassert-delay-ms' to the driver which should now be
removed:
/*
* Note this is a kernel internal device-property set by x86 platform code,
* this MUST not be used in devicetree files without first adding it to
* the DT bindings.
*/
Johan
^ permalink raw reply
* [PATCH] drivers/platform/x86/touchscreen_dmi.c: Add touch config
From: Phoenix Chen @ 2024-01-26 9:53 UTC (permalink / raw)
To: hdegoede
Cc: ilpo.jarvinen, linux-input, platform-driver-x86, linux-kernel,
Phoenix Chen
Added touch screen info for TECLAST X16 Plus tablet.
Signed-off-by: Phoenix Chen <asbeltogf@gmail.com>
---
drivers/platform/x86/touchscreen_dmi.c | 35 ++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 0c6733772698..7aee5e9ff2b8 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -944,6 +944,32 @@ static const struct ts_dmi_data teclast_tbook11_data = {
.properties = teclast_tbook11_props,
};
+static const struct property_entry teclast_x16_plus_props[] = {
+ PROPERTY_ENTRY_U32("touchscreen-min-x", 8),
+ PROPERTY_ENTRY_U32("touchscreen-min-y", 14),
+ PROPERTY_ENTRY_U32("touchscreen-size-x", 1916),
+ PROPERTY_ENTRY_U32("touchscreen-size-y", 1264),
+ PROPERTY_ENTRY_BOOL("touchscreen-inverted-y"),
+ PROPERTY_ENTRY_STRING("firmware-name", "gsl3692-teclast-x16-plus.fw"),
+ PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+ PROPERTY_ENTRY_BOOL("silead,home-button"),
+ { }
+};
+
+static const struct ts_dmi_data teclast_x16_plus_data = {
+ .embedded_fw = {
+ .name = "silead/gsl3692-teclast-x16-plus.fw",
+ .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+ .length = 43560,
+ .sha256 = { 0x9d, 0xb0, 0x3d, 0xf1, 0x00, 0x3c, 0xb5, 0x25,
+ 0x62, 0x8a, 0xa0, 0x93, 0x4b, 0xe0, 0x4e, 0x75,
+ 0xd1, 0x27, 0xb1, 0x65, 0x3c, 0xba, 0xa5, 0x0f,
+ 0xcd, 0xb4, 0xbe, 0x00, 0xbb, 0xf6, 0x43, 0x29 },
+ },
+ .acpi_name = "MSSL1680:00",
+ .properties = teclast_x16_plus_props,
+};
+
static const struct property_entry teclast_x3_plus_props[] = {
PROPERTY_ENTRY_U32("touchscreen-size-x", 1980),
PROPERTY_ENTRY_U32("touchscreen-size-y", 1500),
@@ -1612,6 +1638,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
DMI_MATCH(DMI_PRODUCT_SKU, "E5A6_A1"),
},
},
+ {
+ /* Teclast X16 Plus */
+ .driver_data = (void *)&teclast_x16_plus_data,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "TECLAST"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
+ DMI_MATCH(DMI_PRODUCT_SKU, "D3A5_A1"),
+ },
+ },
{
/* Teclast X3 Plus */
.driver_data = (void *)&teclast_x3_plus_data,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 2/3] HID: bpf: actually free hdev memory after attaching a HID-BPF program
From: Benjamin Tissoires @ 2024-01-26 11:20 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Dan Carpenter, Daniel Borkmann, Andrii Nakryiko,
linux-input, linux-kernel, bpf, stable
In-Reply-To: <20240124-b4-hid-bpf-fixes-v2-2-052520b1e5e6@kernel.org>
On Wed, Jan 24, 2024 at 12:27 PM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> Turns out that I got my reference counts wrong and each successful
> bus_find_device() actually calls get_device(), and we need to manually
> call put_device().
>
> Ensure each bus_find_device() gets a matching put_device() when releasing
> the bpf programs and fix all the error paths.
>
> Cc: stable@vger.kernel.org
> Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> new in v2
> ---
> drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++------
> drivers/hid/bpf/hid_bpf_jmp_table.c | 19 ++++++++++++++++---
> 2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 5111d1fef0d3..7903c8638e81 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -292,7 +292,7 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
> struct hid_device *hdev;
> struct bpf_prog *prog;
> struct device *dev;
> - int fd;
> + int err, fd;
>
> if (!hid_bpf_ops)
> return -EINVAL;
> @@ -311,14 +311,24 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
> * on errors or when it'll be detached
> */
> prog = bpf_prog_get(prog_fd);
> - if (IS_ERR(prog))
> - return PTR_ERR(prog);
> + if (IS_ERR(prog)) {
> + err = PTR_ERR(prog);
> + goto out_dev_put;
> + }
>
> fd = do_hid_bpf_attach_prog(hdev, prog_fd, prog, flags);
> - if (fd < 0)
> - bpf_prog_put(prog);
> + if (fd < 0) {
> + err = fd;
> + goto out_prog_put;
> + }
>
> return fd;
> +
> + out_prog_put:
> + bpf_prog_put(prog);
> + out_dev_put:
> + put_device(dev);
> + return err;
> }
>
> /**
> @@ -345,8 +355,10 @@ hid_bpf_allocate_context(unsigned int hid_id)
> hdev = to_hid_device(dev);
>
> ctx_kern = kzalloc(sizeof(*ctx_kern), GFP_KERNEL);
> - if (!ctx_kern)
> + if (!ctx_kern) {
> + put_device(dev);
> return NULL;
> + }
>
> ctx_kern->ctx.hid = hdev;
>
> @@ -363,10 +375,15 @@ noinline void
> hid_bpf_release_context(struct hid_bpf_ctx *ctx)
> {
> struct hid_bpf_ctx_kern *ctx_kern;
> + struct hid_device *hid;
>
> ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
> + hid = (struct hid_device *)ctx_kern->ctx.hid; /* ignore const */
>
> kfree(ctx_kern);
> +
> + /* get_device() is called by bus_find_device() */
> + put_device(&hid->dev);
> }
>
> /**
> diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
> index 12f7cebddd73..85a24bc0ea25 100644
> --- a/drivers/hid/bpf/hid_bpf_jmp_table.c
> +++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
> @@ -196,6 +196,7 @@ static void __hid_bpf_do_release_prog(int map_fd, unsigned int idx)
> static void hid_bpf_release_progs(struct work_struct *work)
> {
> int i, j, n, map_fd = -1;
> + bool hdev_destroyed;
>
> if (!jmp_table.map)
> return;
> @@ -220,6 +221,12 @@ static void hid_bpf_release_progs(struct work_struct *work)
> if (entry->hdev) {
> hdev = entry->hdev;
> type = entry->type;
> + /*
> + * hdev is still valid, even if we are called after hid_destroy_device():
> + * when hid_bpf_attach() gets called, it takes a ref on the dev through
> + * bus_find_device()
> + */
> + hdev_destroyed = hdev->bpf.destroyed;
>
> hid_bpf_populate_hdev(hdev, type);
>
> @@ -232,12 +239,18 @@ static void hid_bpf_release_progs(struct work_struct *work)
> if (test_bit(next->idx, jmp_table.enabled))
> continue;
>
> - if (next->hdev == hdev && next->type == type)
> + if (next->hdev == hdev && next->type == type) {
> + /*
> + * clear the hdev reference and decrement the device ref
> + * that was taken during bus_find_device() while calling
> + * hid_bpf_attach()
> + */
> next->hdev = NULL;
> + put_device(&hdev->dev);
sigh... I can't make a correct patch these days... Missing a '}' here
to match the open bracket added above :(
I had some debug information put there to check if the device was
actually freed, and the closing bracket got lost while cleaning this
up.
Cheers,
Benjamin
> }
>
> - /* if type was rdesc fixup, reconnect device */
> - if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP)
> + /* if type was rdesc fixup and the device is not gone, reconnect device */
> + if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP && !hdev_destroyed)
> hid_bpf_reconnect(hdev);
> }
> }
>
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v6 2/3] dt-bindings: input: Add TouchNetix axiom touchscreen
From: Krzysztof Kozlowski @ 2024-01-26 11:46 UTC (permalink / raw)
To: Kamel Bouhara, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Henrik Rydberg, linux-input, linux-kernel,
devicetree, Marco Felsch, Jeff LaBundy
Cc: catalin.popescu, mark.satterthwaite, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20240125165823.996910-3-kamel.bouhara@bootlin.com>
On 25/01/2024 17:58, Kamel Bouhara wrote:
> + reset-gpios:
> + maxItems: 1
> +
> + vdda-supply:
> + description: Analog power supply regulator on VDDA pin
> +
> + vddi-supply:
> + description: I/O power supply regulator on VDDI pin
> +
> + startup-time-ms:
> + description: delay after power supply regulator is applied in ms
That's a regulator property - ramp up time.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Daniel Thompson @ 2024-01-26 13:02 UTC (permalink / raw)
To: Johan Hovold
Cc: Bjorn Andersson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Benjamin Tissoires,
Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Johan Hovold,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <ZbNpdaSyFS9tYrkd@hovoldconsulting.com>
On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > The failing read-test in __i2c_hid_core_probe() determines that there's
> > nothing connected at the documented address of the touchscreen.
> >
> > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > ACPI tables. Also wire up the reset-gpio, for good measure.
>
> As the supplies for the touchscreen are always on (and left on by the
> bootloader) it would seem that it is really the addition of the reset
> gpio which makes things work here. Unless the delay is needed for some
> other reason.
>
> (The power-on delay also looks a bit short compared to what is used for
> other devices.)
>
> Reset support was only recently added with commit 2be404486c05 ("HID:
> i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> backport this one before first determining that.
This comment attracted my attention so I tried booting with each of the
three lines individually.
On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> + reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
This is not enough, on it's own, to get the touch screen running.
I guess that's not so much of a surprise since the rebind-the-driver
from userspace trick wouldn't have been touching this reset.
> + post-power-on-delay-ms = <5>;
This line alone is enough (in v6.7.1).
> + post-reset-deassert-delay-ms = <200>;
This line alone is also enough!
In short it looks like the delays make the difference and, even a short
delay, can fix the problem.
Of course, regardless of the line-by-line results I also ran with all
the changes so, FWIW:
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: PS/2 keyboard of laptop Dell XPS 13 9360 goes missing after S3
From: Hans de Goede @ 2024-01-26 13:32 UTC (permalink / raw)
To: Paul Menzel
Cc: linux-input, linux-pm, Dell.Client.Kernel, regressions,
linux-kernel
In-Reply-To: <f07333d2-ebb0-4531-a396-8fb3d1daa2c3@molgen.mpg.de>
Hi Paul,
On 1/26/24 08:03, Paul Menzel wrote:
> Dear Hans,
>
>
> Thank you for your reply, and sorry for the delay on my side. I needed to set up an environment to easily build the Linux kernel.
No problem thank you for testing this.
> Am 22.01.24 um 14:43 schrieb Hans de Goede:
>
>> On 1/21/24 15:26, Paul Menzel wrote:
>
> […]
>
>>> Am 20.01.24 um 21:26 schrieb Hans de Goede:
>>>
>>>> On 1/18/24 13:57, Paul Menzel wrote:
>>>>> #regzbot introduced v6.6.11..v6.7
>>>
>>>>> There seems to be a regression in Linux 6.7 on the Dell XPS 13 9360 (Intel i7-7500U).
>>>>>
>>>>> [ 0.000000] DMI: Dell Inc. XPS 13 9360/0596KF, BIOS 2.21.0 06/02/2022
>>>>>
>>>>> The PS/2 keyboard goes missing after S3 resume¹. The problem does not happen with Linux 6.6.11.
>>>>
>>>> Thank you for reporting this.
>>>>
>>>> Can you try adding "i8042.dumbkbd=1" to your kernel commandline?
>>>>
>>>> This should at least lead to the device not disappearing from
>>>>
>>>> "sudo libinput list-devices"
>>>>
>>>> The next question is if the keyboard will still actually
>>>> work after suspend/resume with "i8042.dumbkbd=1". If it
>>>> stays in the list, but no longer works then there is
>>>> a problem with the i8042 controller; or interrupt
>>>> delivery to the i8042 controller.
>>>>
>>>> If "i8042.dumbkbd=1" somehow fully fixes things, then I guess
>>>> my atkbd driver fix for other laptop keyboards is somehow
>>>> causing issues for yours.
>>>
>>> Just a quick feedback, that booting with `i8042.dumbkbd=1` seems to fix the issue.
>>>
>>>> If "i8042.dumbkbd=1" fully fixes things, can you try building
>>>> your own 6.7.0 kernel with commit 936e4d49ecbc:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=936e4d49ecbc8c404790504386e1422b599dec39
>>>>
>>>> reverted?
>>>
>>> I am going to try that as soon as possible.
>>
>> Assuming this was not some one time glitch with 6.7.0,
>> I have prepared a patch hopefully fixing this (1) as well
>> as a follow up fix to address another potential issue which
>> I have noticed.
>
> Unfortunately, it wasn’t just a glitch.
>
>> Can you please give a 6.7.0 (2) kernel with the 2 attached
>> patches added a try ?
>>
>> I know building kernels can be a bit of work / takes time,
>> sorry. If you are short on time I would prefer testing these 2
>> patches and see if they fix things over trying a plain revert.
>
> Applying both patches on v6.7.1
>
> $ git log --oneline -3
> 053fa44c0de1 (HEAD -> v6.7.1) Input: atkbd - Do not skip atkbd_deactivate() when skipping ATKBD_CMD_GETID
> 0e0fa0113c7a Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
> a91fdae50a6d (tag: v6.7.1, stable/linux-6.7.y, origin/linux-6.7.y) Linux 6.7.1
>
> I am unable to reproduce the problem in eight ACPI S3 suspend/resume cycles. The DMAR errors [3] are also gone:
Thanks.
So thinking more about this I think the DMAR errors are actually the real cause of the issue here, specifically if we replace: f0 with 00 (I guess DMAR uses the high bits for its own purposes) in
`[INTR-REMAP] Request device [f0:1f.0] fault index 0x0`
then the device ID is 00:1f.0 which is the ISA bridge and [INTR-REMAP] errors are known to disable interrupts. The PS/2 controller (which sits behind the ISA bridge) interrupt getting disabled would explain the suspend/resume keyboard issue better then the atkbd.c changes I have been focusing on.
So then the question becomes why does the 6.7.1 kernel not show the DMAR errors. I don't see anything between 6.7.0 and 6.7.1 which explains this. But maybe your local build is using a different configuration which explains this.
Can you try your local 6.7.1 build without my 2 patches? The quickest way to do that would be to run: "git reset --hard HEAD~2" and then re-run the make commandos, this will re-use your previous build so it should be pretty quick.
If things still work after that then the problem is not with the atkbd code.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Johan Hovold @ 2024-01-26 14:31 UTC (permalink / raw)
To: Daniel Thompson
Cc: Bjorn Andersson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Benjamin Tissoires,
Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Johan Hovold,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240126130232.GA5506@aspen.lan>
On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > nothing connected at the documented address of the touchscreen.
> > >
> > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > ACPI tables. Also wire up the reset-gpio, for good measure.
> >
> > As the supplies for the touchscreen are always on (and left on by the
> > bootloader) it would seem that it is really the addition of the reset
> > gpio which makes things work here. Unless the delay is needed for some
> > other reason.
> >
> > (The power-on delay also looks a bit short compared to what is used for
> > other devices.)
> >
> > Reset support was only recently added with commit 2be404486c05 ("HID:
> > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > backport this one before first determining that.
>
> This comment attracted my attention so I tried booting with each of the
> three lines individually.
>
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > + reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
>
> This is not enough, on it's own, to get the touch screen running.
>
> I guess that's not so much of a surprise since the rebind-the-driver
> from userspace trick wouldn't have been touching this reset.
Right, I realised that after hitting send.
For the record, people have successfully been using the touchpad after
forcing the driver to reprobe through sysfs:
echo 4-0010 >/sys/bus/i2c/drivers/i2c_hid_of/bind
> > + post-power-on-delay-ms = <5>;
>
> This line alone is enough (in v6.7.1).
Thanks for confirming.
> > + post-reset-deassert-delay-ms = <200>;
>
> This line alone is also enough!
Yes, the driver honours this delay regardless of whether a reset gpio is
defined currently, so this is expected.
> In short it looks like the delays make the difference and, even a short
> delay, can fix the problem.
Right, but since the suppliers are left enabled by the bootloader (and
never disabled by the kernel), that only begs the question of why this
makes a difference.
Without the delay, the other HID devices are probing (successfully)
slightly before, but essentially in parallel with the touchscreen while
using the same resources. Is that causing trouble somehow?
Or is there a bug in the i2c controller driver affecting only this
device that can be worked around by adding a delay before the first
transfer?
Johan
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-26 14:53 UTC (permalink / raw)
To: Johan Hovold
Cc: Daniel Thompson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Benjamin Tissoires,
Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Johan Hovold,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <ZbPCJv7HW8OQzPMT@hovoldconsulting.com>
On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> > On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > > nothing connected at the documented address of the touchscreen.
> > > >
> > > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > > ACPI tables. Also wire up the reset-gpio, for good measure.
> > >
> > > As the supplies for the touchscreen are always on (and left on by the
> > > bootloader) it would seem that it is really the addition of the reset
> > > gpio which makes things work here. Unless the delay is needed for some
> > > other reason.
> > >
> > > (The power-on delay also looks a bit short compared to what is used for
> > > other devices.)
> > >
> > > Reset support was only recently added with commit 2be404486c05 ("HID:
> > > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > > backport this one before first determining that.
> >
> > This comment attracted my attention so I tried booting with each of the
> > three lines individually.
> >
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > + reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> >
> > This is not enough, on it's own, to get the touch screen running.
> >
> > I guess that's not so much of a surprise since the rebind-the-driver
> > from userspace trick wouldn't have been touching this reset.
>
> Right, I realised that after hitting send.
>
> For the record, people have successfully been using the touchpad after
> forcing the driver to reprobe through sysfs:
>
> echo 4-0010 >/sys/bus/i2c/drivers/i2c_hid_of/bind
>
> > > + post-power-on-delay-ms = <5>;
> >
> > This line alone is enough (in v6.7.1).
>
> Thanks for confirming.
>
> > > + post-reset-deassert-delay-ms = <200>;
> >
> > This line alone is also enough!
>
> Yes, the driver honours this delay regardless of whether a reset gpio is
> defined currently, so this is expected.
>
> > In short it looks like the delays make the difference and, even a short
> > delay, can fix the problem.
>
> Right, but since the suppliers are left enabled by the bootloader (and
> never disabled by the kernel), that only begs the question of why this
> makes a difference.
>
You're right, the supply is kept on by other things, so this isn't the
problem.
> Without the delay, the other HID devices are probing (successfully)
> slightly before, but essentially in parallel with the touchscreen while
> using the same resources. Is that causing trouble somehow?
>
The difference to those other HID devices is GPIO 99 - the reset pin,
which is configured pull down input from boot - i.e. the chip is held in
reset.
When the HID device is being probed, pinctrl applies &ts0_default starts
driving it high, bringing the device out of reset. But insufficient time
is given for the chip to come up so the I2C read fails.
If you later try to probe again, 200ms has elapsed since the reset was
deasserted (driven high).
Regards,
Bjorn
> Or is there a bug in the i2c controller driver affecting only this
> device that can be worked around by adding a delay before the first
> transfer?
>
> Johan
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-26 15:05 UTC (permalink / raw)
To: Daniel Thompson
Cc: Johan Hovold, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Benjamin Tissoires, Jiri Kosina, Bjorn Andersson,
Konrad Dybcio, Johan Hovold, linux-arm-msm, linux-input,
devicetree, linux-kernel, Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240126130232.GA5506@aspen.lan>
On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > nothing connected at the documented address of the touchscreen.
> > >
> > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > ACPI tables. Also wire up the reset-gpio, for good measure.
> >
> > As the supplies for the touchscreen are always on (and left on by the
> > bootloader) it would seem that it is really the addition of the reset
> > gpio which makes things work here. Unless the delay is needed for some
> > other reason.
> >
> > (The power-on delay also looks a bit short compared to what is used for
> > other devices.)
> >
> > Reset support was only recently added with commit 2be404486c05 ("HID:
> > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > backport this one before first determining that.
>
> This comment attracted my attention so I tried booting with each of the
> three lines individually.
>
>
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > + reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
>
> This is not enough, on it's own, to get the touch screen running.
>
No, because pinctrl already brings the chip out of reset without this.
> I guess that's not so much of a surprise since the rebind-the-driver
> from userspace trick wouldn't have been touching this reset.
>
Right, it would just have been left deasserted from the first attempt.
That said, the addition of the reset means that we're now asserting
reset in such rebind attempts. And as such the
post-reset-deassert-delay-ms is now needed between the explicit deassert
from the driver and the i2c read.
>
> > + post-power-on-delay-ms = <5>;
>
> This line alone is enough (in v6.7.1).
>
So the delay taken through really_probe() until we reach that i2c read
is almost the entire delay needed, on your specific device.
>
> > + post-reset-deassert-delay-ms = <200>;
>
> This line alone is also enough!
>
> In short it looks like the delays make the difference and, even a short
> delay, can fix the problem.
>
> Of course, regardless of the line-by-line results I also ran with all
> the changes so, FWIW:
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
>
Thanks,
Bjorn
>
> Daniel.
^ permalink raw reply
* Re: PS/2 keyboard of laptop Dell XPS 13 9360 goes missing after S3
From: Hans de Goede @ 2024-01-26 15:58 UTC (permalink / raw)
To: Paul Menzel
Cc: linux-input, linux-pm, Dell.Client.Kernel, regressions,
linux-kernel
In-Reply-To: <5f70a174-7f18-43c0-b3a3-b72544a2631b@redhat.com>
Hi Paul,
On 1/26/24 14:32, Hans de Goede wrote:
> Hi Paul,
>
> On 1/26/24 08:03, Paul Menzel wrote:
>> Dear Hans,
>>
>>
>> Thank you for your reply, and sorry for the delay on my side. I needed to set up an environment to easily build the Linux kernel.
>
> No problem thank you for testing this.
>
>> Am 22.01.24 um 14:43 schrieb Hans de Goede:
>>
>>> On 1/21/24 15:26, Paul Menzel wrote:
>>
>> […]
>>
>>>> Am 20.01.24 um 21:26 schrieb Hans de Goede:
>>>>
>>>>> On 1/18/24 13:57, Paul Menzel wrote:
>>>>>> #regzbot introduced v6.6.11..v6.7
>>>>
>>>>>> There seems to be a regression in Linux 6.7 on the Dell XPS 13 9360 (Intel i7-7500U).
>>>>>>
>>>>>> [ 0.000000] DMI: Dell Inc. XPS 13 9360/0596KF, BIOS 2.21.0 06/02/2022
>>>>>>
>>>>>> The PS/2 keyboard goes missing after S3 resume¹. The problem does not happen with Linux 6.6.11.
>>>>>
>>>>> Thank you for reporting this.
>>>>>
>>>>> Can you try adding "i8042.dumbkbd=1" to your kernel commandline?
>>>>>
>>>>> This should at least lead to the device not disappearing from
>>>>>
>>>>> "sudo libinput list-devices"
>>>>>
>>>>> The next question is if the keyboard will still actually
>>>>> work after suspend/resume with "i8042.dumbkbd=1". If it
>>>>> stays in the list, but no longer works then there is
>>>>> a problem with the i8042 controller; or interrupt
>>>>> delivery to the i8042 controller.
>>>>>
>>>>> If "i8042.dumbkbd=1" somehow fully fixes things, then I guess
>>>>> my atkbd driver fix for other laptop keyboards is somehow
>>>>> causing issues for yours.
>>>>
>>>> Just a quick feedback, that booting with `i8042.dumbkbd=1` seems to fix the issue.
>>>>
>>>>> If "i8042.dumbkbd=1" fully fixes things, can you try building
>>>>> your own 6.7.0 kernel with commit 936e4d49ecbc:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=936e4d49ecbc8c404790504386e1422b599dec39
>>>>>
>>>>> reverted?
>>>>
>>>> I am going to try that as soon as possible.
>>>
>>> Assuming this was not some one time glitch with 6.7.0,
>>> I have prepared a patch hopefully fixing this (1) as well
>>> as a follow up fix to address another potential issue which
>>> I have noticed.
>>
>> Unfortunately, it wasn’t just a glitch.
>>
>>> Can you please give a 6.7.0 (2) kernel with the 2 attached
>>> patches added a try ?
>>>
>>> I know building kernels can be a bit of work / takes time,
>>> sorry. If you are short on time I would prefer testing these 2
>>> patches and see if they fix things over trying a plain revert.
>>
>> Applying both patches on v6.7.1
>>
>> $ git log --oneline -3
>> 053fa44c0de1 (HEAD -> v6.7.1) Input: atkbd - Do not skip atkbd_deactivate() when skipping ATKBD_CMD_GETID
>> 0e0fa0113c7a Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
>> a91fdae50a6d (tag: v6.7.1, stable/linux-6.7.y, origin/linux-6.7.y) Linux 6.7.1
>>
>> I am unable to reproduce the problem in eight ACPI S3 suspend/resume cycles. The DMAR errors [3] are also gone:
>
> Thanks.
>
> So thinking more about this I think the DMAR errors are actually the real cause of the issue here, specifically if we replace: f0 with 00 (I guess DMAR uses the high bits for its own purposes) in
>
> `[INTR-REMAP] Request device [f0:1f.0] fault index 0x0`
>
> then the device ID is 00:1f.0 which is the ISA bridge and [INTR-REMAP] errors are known to disable interrupts. The PS/2 controller (which sits behind the ISA bridge) interrupt getting disabled would explain the suspend/resume keyboard issue better then the atkbd.c changes I have been focusing on.
>
> So then the question becomes why does the 6.7.1 kernel not show the DMAR errors. I don't see anything between 6.7.0 and 6.7.1 which explains this. But maybe your local build is using a different configuration which explains this.
>
> Can you try your local 6.7.1 build without my 2 patches? The quickest way to do that would be to run: "git reset --hard HEAD~2" and then re-run the make commandos, this will re-use your previous build so it should be pretty quick.
>
> If things still work after that then the problem is not with the atkbd code.
Never mind I just became aware of a bunch of other reports which don't have
the DMAR errors on resume, so I'm submitting the 2 fixes for this upstream
now.
Regards,
Hans
^ permalink raw reply
* [PATCH regression fix 1/2] Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
From: Hans de Goede @ 2024-01-26 16:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Menzel, stable, regressions, linux-input
In-Reply-To: <20240126160724.13278-1-hdegoede@redhat.com>
After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
translated mode") the keyboard on Dell XPS 13 9350 / 9360 / 9370 models
has stopped working after a suspend/resume.
The problem appears to be that atkbd_probe() fails when called
from atkbd_reconnect() on resume, which on systems where
ATKBD_CMD_GETID is skipped can only happen by ATKBD_CMD_SETLEDS
failing. ATKBD_CMD_SETLEDS failing because ATKBD_CMD_GETID was
skipped is weird, but apparently that is what is happening.
Fix this by also skipping ATKBD_CMD_SETLEDS when skipping
ATKBD_CMD_GETID.
Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/linux-input/0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de/
Closes: https://bbs.archlinux.org/viewtopic.php?pid=2146300
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218424
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2260517
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/keyboard/atkbd.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 13ef6284223d..c229bd6b3f7f 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -811,7 +811,6 @@ static int atkbd_probe(struct atkbd *atkbd)
{
struct ps2dev *ps2dev = &atkbd->ps2dev;
unsigned char param[2];
- bool skip_getid;
/*
* Some systems, where the bit-twiddling when testing the io-lines of the
@@ -825,6 +824,11 @@ static int atkbd_probe(struct atkbd *atkbd)
"keyboard reset failed on %s\n",
ps2dev->serio->phys);
+ if (atkbd_skip_getid(atkbd)) {
+ atkbd->id = 0xab83;
+ return 0;
+ }
+
/*
* Then we check the keyboard ID. We should get 0xab83 under normal conditions.
* Some keyboards report different values, but the first byte is always 0xab or
@@ -833,18 +837,17 @@ static int atkbd_probe(struct atkbd *atkbd)
*/
param[0] = param[1] = 0xa5; /* initialize with invalid values */
- skip_getid = atkbd_skip_getid(atkbd);
- if (skip_getid || ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
+ if (ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
/*
- * If the get ID command was skipped or failed, we check if we can at least set
+ * If the get ID command failed, we check if we can at least set
* the LEDs on the keyboard. This should work on every keyboard out there.
* It also turns the LEDs off, which we want anyway.
*/
param[0] = 0;
if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
return -1;
- atkbd->id = skip_getid ? 0xab83 : 0xabba;
+ atkbd->id = 0xabba;
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression
From: Hans de Goede @ 2024-01-26 16:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Menzel, stable, regressions, linux-input
Hi Dmitry,
There have been multiple reports that the keyboard on
Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
atkbd - skip ATKBD_CMD_GETID in translated mode").
See the 4 closes tags in the first patch for 4 reports of this.
I have been working with the first reporter on resolving this
and testing on his Dell XPS 13 9360 confirms that these patches
fix things.
Unfortunately the commit causing the issue has also been picked
up by multiple stable kernel series now. Can you please send
these fixes to Linus ASAP, so that they can also be backported
to the stable series ASAP ?
Alternatively we could revert the commit causing this, but that
commit is know to fix issues on a whole bunch of other laptops
so I would rather not revert it.
Regards,
Hans
Hans de Goede (2):
Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
Input: atkbd - Do not skip atkbd_deactivate() when skipping
ATKBD_CMD_GETID
drivers/input/keyboard/atkbd.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--
2.43.0
^ 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