On Fri, Oct 20, 2023 at 02:03:10PM +0200, Marco Felsch wrote:
> Hi Kamel,
>
Hi Marco,
> just a rough review.
Thanks !
[...]
> > +#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/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
>
> Can you please check if all headers are required e.g. sting.h
> seems a bit suspicious here.
Sure, slab.h and string.h are no more required.
>
> > +/*
> > + * Runtime TCP mode: device is executing normal code and is
> > + * accessible via the Touch Controller Mode
> > + */
> > +#define BOOT_TCP 0
> > +/*
> > + * Bootloader BLP mode: device is executing bootloader and is
> > + * accessible via the Boot Loader Protocol.
> > + */
> > +#define BOOT_BLP 1
>
> Both defines are not used.
>
Ack.
> > +#define AXIOM_PROX_LEVEL -128
> > +/*
> > + * Register group u31 has 2 pages for usage table entries.
> > + * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
i> > + */
> > +#define AXIOM_U31_MAX_USAGES 85
>
> The programmer's guid describe the usage as hexadecimal number prefixed
> with an 'u'. The current range is from u00 till uFF, so the max. usages
> should be 0xff.
Based one the above comment, it seems we are computing the byte size of
an usage array. I agree this might require to be more clear though.
>
> > +#define AXIOM_U31_BYTES_PER_USAGE 6
> > +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> > +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> > +#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
> > +#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
> > +
> > +#define AXIOM_U41_MAX_TARGETS 10
> > +
> > +#define AXIOM_U46_AUX_CHANNELS 4
> > +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
>
> I'm still not very happy with the decoding, since the so called
> 'protocol' is clear and versioned we can add the all required protocols
> as struct which has far less magic offsets.
Im not sure it will really make a significant difference as we actually
ihave a limited set of registers for the i2c driver, also could you
please clarify what protocol your refering to here ?
>
> > +
> > +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> > +#define AXIOM_COMMS_PAGE_SIZE 256
> > +#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
> > +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
> > +
> > +#define AXIOM_REBASELINE_CMD 0x03
> > +
> > +#define AXIOM_REPORT_USAGE_ID 0x34
> > +#define AXIOM_DEVINFO_USAGE_ID 0x31
> > +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> > +#define AXIOM_REBASELINE_USAGE_ID 0x02
> > +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> > +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> > +
> > +#define AXIOM_PAGE_MASK GENMASK(15, 8)
>
> Unused
Ack thx.
[...]
> > +/*
> > + * Holds state of a touch or target when detected prior a touch (eg.
> > + * hover or proximity events).
> > + */
> > +enum axiom_target_state {
> > + TARGET_STATE_NOT_PRESENT = 0,
> > + TARGET_STATE_PROX = 1,
> > + TARGET_STATE_HOVER = 2,
> > + TARGET_STATE_TOUCHING = 3,
> > + TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
> > + TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
>
> STATE_MIN/MAX not used.
Ack.
>
> > +};
> > +
> > +struct 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 {
> > + u16 target_address;
> > + u16 length:15;
> > + u16 read:1;
> > +} __packed;
> > +
> > +struct axiom_data {
> > + struct axiom_devinfo devinfo;
> > + struct device *dev;
> > + struct gpio_desc *reset_gpio;
> > + struct gpio_desc *irq_gpio;
>
> No need to store the irq_gpio here.
>
Right, thanks.
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + u32 max_report_len;
> > + u32 report_overflow_counter;
> > + u32 report_counter;
> > + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> > + struct u41_target targets[AXIOM_U41_MAX_TARGETS];
> > + struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> > + bool usage_table_populated;
> > +};
> > +
> > +/*
> > + * aXiom devices are typically configured to report
> > + * touches at a rate of 100Hz (10ms). For systems
> > + * that require polling for reports, 100ms seems like
> > + * an acceptable polling rate.
> > + * 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 100
>
> Above you describe that the touch-rate is ~10ms why do we configure it
> 10-times lower here? Also 100ms is huge if you think about user respone
> time.
I am not completely sure aboud this yet, here 100ms is based on my own
*limited* experience, I agree we should stick to the 10ms.
>
> > +/* Translate usage/page/offset triplet into physical address. */
> > +static u16
> > +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > + char offset)
> > +{
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > + u32 i;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + /* 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;
> > + }
> > +
> > + for (i = 0; i < device_info->num_usages; i++) {
> > + if (usage_table[i].id != usage)
> > + continue;
> > +
> > + if (page >= usage_table[i].num_pages) {
> > + dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> > + usage, page, offset);
> > + return 0xffff;
> > + }
> > + }
>
> We can avoid this loop if we store the usage table exactly as it is,
> e.g.:
>
> usage_table[0x31] = u31;
> usage_table[0x41] = u41;
>
Could you please explain your idea ?
> > + return ((usage_table[i].start_page + page) << 8) + offset;
> > +}
> > +
> > +static int
> > +axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > + struct axiom_cmd_header cmd_header;
> > + struct i2c_msg msg[2];
> > + int ret;
> > +
> > + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > + cmd_header.length = cpu_to_le16(len);
> > + cmd_header.read = 1;
> > +
> > + msg[0].addr = client->addr;
> > + msg[0].flags = 0;
> > + msg[0].len = sizeof(cmd_header);
> > + msg[0].buf = (u8 *)&cmd_header;
> > +
> > + msg[1].addr = client->addr;
> > + msg[1].flags = I2C_M_RD;
> > + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
> > +
> > + ret = i2c_transfer(client->adapter, msg, 2);
> > + if (ret != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev,
> > + "Failed reading usage %#x page %#x, error=%d\n",
> > + usage, page, ret);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > + struct axiom_cmd_header cmd_header;
> > + struct i2c_msg msg[2];
> > + int ret;
> > +
> > + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > + cmd_header.length = cpu_to_le16(len);
> > + cmd_header.read = 0;
> > +
> > + msg[0].addr = client->addr;
> > + msg[0].flags = 0;
> > + msg[0].len = sizeof(cmd_header);
> > + msg[0].buf = (u8 *)&cmd_header;
> > +
> > + msg[1].addr = client->addr;
> > + msg[1].flags = 0;
> > + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
>
> Please check the "comms protocol app note", for write it is not allowed
> to send a stop, so the whole data must be send in one i2c_msg.
>
Well I only have the "Programmer's Guide", I'll have to check that as it
really seems to works as it yet.
> > +
> > + ret = i2c_transfer(client->adapter, msg, 2);
> > + if (ret < 0) {
> > + dev_err(&client->dev,
> > + "Failed to write usage %#x page %#x, error=%d\n", usage,
> > + page, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Decodes and populates the local Usage Table.
> > + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> > + * device.
> > + */
> > +static u32 axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> > +{
> > + u32 usage_id = 0;
> > + u32 max_report_len = 0;
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> > + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> > + char id = rx_data[offset + 0];
> > + char start_page = rx_data[offset + 1];
> > + char 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 %2u Info: %*ph\n", usage_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;
> > + }
>
> As said, the sorting can be really easy:
>
> usage_table[0x01] = u01;
> usage_table[0x31] = u31;
>
I still don't get your point here.
> > + ts->usage_table_populated = true;
> > +
> > + return max_report_len;
> > +}
> > +
[...]
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct input_dev *input_dev;
> > + struct axiom_data *ts;
> > + int ret;
> > + int target;
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > + ts->dev = dev;
> > +
> > + ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
> > + if (IS_ERR(ts->irq_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq GPIO");
>
> Nope you removed this from the binding.
Yes, will be fixed in v4.
>
> > + 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");
>
> Please also add a regulator for the VDDI/VDDA which is required for the
> device to work properly.
>
Right, Im using the AX54 EV board with fixed regulators.
> > + axiom_reset(ts->reset_gpio);
> > +
> > + if (ts->irq_gpio) {
>
> Nope, please drop the ts->irq_gpio check.
Ack.
>
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + axiom_irq, 0, dev_name(dev), ts);
> > + if (ret < 0)
>
> If the threaded_irq does fail you can fallback to the polling mode.
Indeed.
>
> > + return dev_err_probe(dev, ret, "Failed to request threaded IRQ\n");
> > + }
> > +
> > + ret = axiom_discover(ts);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
> > +
> > + ret = axiom_rebaseline(ts);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed touchscreen re-baselining\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);
> > +
> > + /* Multi Touch */
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + 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);
> > +
> > + /* Registers the axiom device as a touchscreen instead of as a mouse pointer */
> > + input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > +
> > + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> > +
> > + /* 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);
> > +
> > + if (!ts->irq_gpio) {
> > + ret = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (ret)
> > + return dev_err_probe(ts->dev, ret, "Unable to set up polling mode\n");
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + 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 = TARGET_STATE_NOT_PRESENT;
> > +
> > + ret = input_register_device(input_dev);
> > +
> > + if (ret)
> > + return dev_err_probe(ts->dev, ret,
> > + "Could not register with Input Sub-system.\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_remove(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > +
> > + input_unregister_device(ts->input_dev);
>
> This can be part of devm_add_action_or_reset() and we could remove the
> .remove() callback for this driver.
>
Sure, thanks for the tips :)!
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + { "axiom-ax54a" },
>
> Albeit the datasheet says: "axiom ax54a" I think ax stands for axiom. So
> the name should be "ax54a" only?
Yes this is actually a good point, we can move to ax54a only.
>
> > + {},
>
> Nit: { },
> > +};
> > +
>
> Please drop the unnecessary newline.
>
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_of_match[] = {
> > + { .compatible = "touchnetix,axiom-ax54a", },
> > + {}
>
> same here.
>
> > +};
> > +
>
> same here.
>
> > +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,
> > + .remove = axiom_i2c_remove,
> > +};
> > +
>
> same here.
>
OK.
> > +module_i2c_driver(axiom_i2c_driver);
> > +
> > +MODULE_AUTHOR("Bart Prescott <bartp@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@xxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
> >
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
