Hi Lee, Thank you for your continued support and thorough review. On Fri, Jan 17, 2020 at 01:21:43PM +0000, Lee Jones wrote: > On Fri, 17 Jan 2020, Jeff LaBundy wrote: > > > This patch adds core support for the Azoteq IQS620A, IQS621, IQS622, > > IQS624 and IQS625 multi-function sensors. > > > > Signed-off-by: Jeff LaBundy <jeff@xxxxxxxxxxx> > > --- > > Changes in v4: > > - None > > > > Changes in v3: > > - None > > > > Changes in v2: > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > - Replaced 'error' with 'ret' throughout > > - Updated iqs62x_dev_init to account for 4/8/16-MHz clock divider in start-up > > delays and replaced ATI timeout routine with regmap_read_poll_timeout > > - Added an error message to iqs62x_irq in case device status fails to be read > > - Replaced sw_num member of iqs62x_core with a local variable in iqs62x_probe > > as the former was unused anywhere else > > - Added comments throughout iqs62x_probe to clarify how devices are matched > > based on the presence of calibration data > > - Inverted the product and software number comparison logic in iqs62x_probe > > to avoid an else...continue branch > > - Changed iqs62x_probe from .probe callback to .probe_new callback, thereby > > eliminating the otherwise unused iqs62x_id array > > - Moved iqs62x_suspend and iqs62x_resume below iqs62x_remove > > - Eliminated tabbed alignment of regmap_config and i2c_driver struct members > > - Added register definitions for register addresses used in iqs621_cal_regs, > > iqs620at_cal_regs and iqs62x_devs arrays > > - Removed of_compatible string from IQS622 mfd_cell struct as its proximity > > (now ambient light) sensing functionality need not be represented using a > > child node > > - Dissolved union in iqs62x_event_data to allow simultaneous use of ir_flags > > and als_flags > > - Removed temp_flags member of iqs62x_event_data, IQS62X_EVENT_TEMP register > > enumeration and IQS62X_EVENT_UI_HI/LO from iqs620a_event_regs (thereby re- > > ducing IQS62X_EVENT_SIZE to 10) as they were unused > > > > drivers/mfd/Kconfig | 13 + > > drivers/mfd/Makefile | 3 + > > drivers/mfd/iqs62x-core.c | 639 ++++++++++++++++++++++++++++++++++++++++++++ > > drivers/mfd/iqs62x-tables.c | 438 ++++++++++++++++++++++++++++++ > > include/linux/mfd/iqs62x.h | 146 ++++++++++ > > 5 files changed, 1239 insertions(+) > > create mode 100644 drivers/mfd/iqs62x-core.c > > create mode 100644 drivers/mfd/iqs62x-tables.c > > create mode 100644 include/linux/mfd/iqs62x.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 4209008..151984c 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -642,6 +642,19 @@ config MFD_IPAQ_MICRO > > AT90LS8535 microcontroller flashed with a special iPAQ > > firmware using the custom protocol implemented in this driver. > > > > +config MFD_IQS62X > > + tristate "Azoteq IQS620A/621/622/624/625 core support" > > + depends on I2C > > + select MFD_CORE > > + select REGMAP_I2C > > + help > > + Say Y here if you want to build core support for the Azoteq IQS620A, > > + IQS621, IQS622, IQS624 and IQS625 multi-function sensors. Additional > > + options must be selected to enable device-specific functions. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called iqs62x. > > + > > config MFD_JANZ_CMODIO > > tristate "Janz CMOD-IO PCI MODULbus Carrier Board" > > select MFD_CORE > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index aed99f0..c4fc26b 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -232,6 +232,9 @@ obj-$(CONFIG_MFD_DLN2) += dln2.o > > obj-$(CONFIG_MFD_RT5033) += rt5033.o > > obj-$(CONFIG_MFD_SKY81452) += sky81452.o > > > > +iqs62x-objs := iqs62x-core.o iqs62x-tables.o > > +obj-$(CONFIG_MFD_IQS62X) += iqs62x.o > > + > > intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o > > obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > > diff --git a/drivers/mfd/iqs62x-core.c b/drivers/mfd/iqs62x-core.c > > new file mode 100644 > > index 0000000..767f9d8 > > --- /dev/null > > +++ b/drivers/mfd/iqs62x-core.c > > @@ -0,0 +1,639 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Azoteq IQS620A/621/622/624/625 Multi-Function Sensors > > + * > > + * Copyright (C) 2019 Jeff LaBundy <jeff@xxxxxxxxxxx> > > + * > > + * These devices rely on application-specific register settings and calibration > > + * data developed in and exported from a suite of GUIs offered by the vendor. A > > + * separate tool converts the GUIs' ASCII-based output into a standard firmware > > + * file parsed by the driver. > > + * > > + * Link to data sheets and GUIs: https://www.azoteq.com/ > > + * > > + * Link to conversion tool: https://github.com/jlabundy/iqs62x-h2bin.git > > + */ > > + > > +#include <linux/completion.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/firmware.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/list.h> > > +#include <linux/mfd/core.h> > > +#include <linux/module.h> > > +#include <linux/notifier.h> > > +#include <linux/of_device.h> > > +#include <linux/property.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > +#include <asm/unaligned.h> > > + > > +#include <linux/mfd/iqs62x.h> > > + > > +#define IQS62X_PROD_NUM 0x00 > > + > > +#define IQS62X_SYS_FLAGS 0x10 > > +#define IQS62X_SYS_FLAGS_IN_ATI BIT(2) > > + > > +#define IQS622_PROX_SETTINGS_4 0x48 > > +#define IQS620_PROX_SETTINGS_4 0x50 > > +#define IQS620_PROX_SETTINGS_4_SAR_EN BIT(7) > > + > > +#define IQS62X_SYS_SETTINGS 0xD0 > > +#define IQS62X_SYS_SETTINGS_SOFT_RESET BIT(7) > > +#define IQS62X_SYS_SETTINGS_ACK_RESET BIT(6) > > +#define IQS62X_SYS_SETTINGS_EVENT_MODE BIT(5) > > +#define IQS62X_SYS_SETTINGS_CLK_DIV BIT(4) > > +#define IQS62X_SYS_SETTINGS_REDO_ATI BIT(1) > > + > > +#define IQS62X_PWR_SETTINGS 0xD2 > > +#define IQS62X_PWR_SETTINGS_DIS_AUTO BIT(5) > > +#define IQS62X_PWR_SETTINGS_PWR_MODE_MASK (BIT(4) | BIT(3)) > > +#define IQS62X_PWR_SETTINGS_PWR_MODE_HALT (BIT(4) | BIT(3)) > > +#define IQS62X_PWR_SETTINGS_PWR_MODE_NORM 0 > > + > > +#define IQS62X_OTP_CMD 0xF0 > > +#define IQS62X_OTP_CMD_FG3 0x13 > > +#define IQS62X_OTP_DATA 0xF1 > > +#define IQS62X_MAX_REG 0xFF > > + > > +#define IQS62X_HALL_CAL_MASK GENMASK(3, 0) > > + > > +#define IQS62X_FW_REC_TYPE_INFO 0 > > +#define IQS62X_FW_REC_TYPE_PROD 1 > > +#define IQS62X_FW_REC_TYPE_HALL 2 > > +#define IQS62X_FW_REC_TYPE_MASK 3 > > +#define IQS62X_FW_REC_TYPE_DATA 4 > > + > > +struct iqs62x_fw_rec { > > + u8 type; > > + u8 addr; > > + u8 len; > > + u8 data; > > +} __packed; > > + > > +struct iqs62x_fw_blk { > > + struct list_head list; > > + u8 addr; > > + u8 mask; > > + u8 len; > > + u8 data[]; > > +}; > > + > > +struct iqs62x_info { > > + u8 prod_num; > > + u8 sw_num; > > + u8 hw_num; > > +} __packed; > > + > > +static int iqs62x_dev_init(struct iqs62x_core *iqs62x) > > +{ > > + struct iqs62x_fw_blk *fw_blk; > > + unsigned int val; > > + int ret; > > + u8 clk_div = 1; > > + > > + list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) { > > + if (fw_blk->mask) > > + ret = regmap_update_bits(iqs62x->map, fw_blk->addr, > > + fw_blk->mask, *fw_blk->data); > > + else > > + ret = regmap_raw_write(iqs62x->map, fw_blk->addr, > > + fw_blk->data, fw_blk->len); > > + if (ret) > > + return ret; > > + } > > + > > + switch (iqs62x->dev_desc->prod_num) { > > + case IQS620_PROD_NUM: > > + case IQS622_PROD_NUM: > > + ret = regmap_read(iqs62x->map, iqs62x->dev_desc->prod_num == > > + IQS620_PROD_NUM ? IQS620_PROX_SETTINGS_4 : > > + IQS622_PROX_SETTINGS_4, > > Not overly taken by this. Can you pull it out please? Sure thing, will do. > > > + &val); > > + if (ret) > > + return ret; > > + > > + if (val & IQS620_PROX_SETTINGS_4_SAR_EN) > > + iqs62x->ui_sel = IQS62X_UI_SAR1; > > + /* fall through */ > > + > > Prefer the '\n' above the comment. Sure thing, will do. > > > + case IQS621_PROD_NUM: > > + ret = regmap_write(iqs62x->map, IQS620_GLBL_EVENT_MASK, > > + IQS620_GLBL_EVENT_MASK_PMU | > > + iqs62x->dev_desc->prox_mask | > > + iqs62x->dev_desc->sar_mask | > > + iqs62x->dev_desc->hall_mask | > > + iqs62x->dev_desc->hyst_mask | > > + iqs62x->dev_desc->temp_mask | > > + iqs62x->dev_desc->als_mask | > > + iqs62x->dev_desc->ir_mask); > > + if (ret) > > + return ret; > > + break; > > + > > + default: > > + ret = regmap_write(iqs62x->map, IQS624_HALL_UI, > > + IQS624_HALL_UI_WHL_EVENT | > > + IQS624_HALL_UI_INT_EVENT | > > + IQS624_HALL_UI_AUTO_CAL); > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(iqs62x->map, IQS624_INTERVAL_DIV, &val); > > + if (ret) > > + return ret; > > + > > + if (val >= iqs62x->dev_desc->interval_div) > > + break; > > I think this would benefit from a comment. Sure thing, will do. > > > + ret = regmap_write(iqs62x->map, IQS624_INTERVAL_DIV, > > + iqs62x->dev_desc->interval_div); > > + if (ret) > > + return ret; > > + } > > + > > + ret = regmap_read(iqs62x->map, IQS62X_SYS_SETTINGS, &val); > > + if (ret) > > + return ret; > > + > > + if (val & IQS62X_SYS_SETTINGS_CLK_DIV) > > + clk_div = iqs62x->dev_desc->clk_div; > > + > > + ret = regmap_write(iqs62x->map, IQS62X_SYS_SETTINGS, val | > > + IQS62X_SYS_SETTINGS_ACK_RESET | > > + IQS62X_SYS_SETTINGS_EVENT_MODE | > > + IQS62X_SYS_SETTINGS_REDO_ATI); > > + if (ret) > > + return ret; > > + > > + ret = regmap_read_poll_timeout(iqs62x->map, IQS62X_SYS_FLAGS, val, > > + !(val & IQS62X_SYS_FLAGS_IN_ATI), > > + 10000, clk_div * 500000); > > Would prefer if you define the magic numbers. Sure thing, will do. > > > + if (ret) > > + return ret; > > + > > + /* > > + * The following delay accommodates the post-ATI stabilization time > > + * specified in the data sheet (with additional margin). > > Nit: "datasheet" Sure thing, will do. > > > + */ > > + msleep(clk_div * 150); > > + > > + return 0; > > +} > > + > > +static int iqs62x_fw_prs(struct iqs62x_core *iqs62x, const struct firmware *fw) > > +{ > > + struct i2c_client *client = iqs62x->client; > > + struct iqs62x_fw_rec *fw_rec; > > + struct iqs62x_fw_blk *fw_blk; > > + unsigned int val; > > + size_t pos = 0; > > + int ret = 0; > > + u8 mask, len, *data; > > + u8 hall_cal_index = 0; > > + > > + while (pos < fw->size) { > > + if (pos + sizeof(*fw_rec) > fw->size) { > > + ret = -EINVAL; > > + break; > > + } > > + fw_rec = (struct iqs62x_fw_rec *)(fw->data + pos); > > + pos += sizeof(*fw_rec); > > + > > + if (pos + fw_rec->len - 1 > fw->size) { > > + ret = -EINVAL; > > + break; > > + } > > + pos += fw_rec->len - 1; > > + > > + switch (fw_rec->type) { > > + case IQS62X_FW_REC_TYPE_INFO: > > + continue; > > + > > + case IQS62X_FW_REC_TYPE_PROD: > > + if (fw_rec->data == iqs62x->dev_desc->prod_num) > > + continue; > > + > > + dev_err(&client->dev, > > + "Incompatible product number: 0x%02X\n", > > + fw_rec->data); > > + ret = -EINVAL; > > + break; > > + > > + case IQS62X_FW_REC_TYPE_HALL: > > + if (!hall_cal_index) { > > + ret = regmap_write(iqs62x->map, IQS62X_OTP_CMD, > > + IQS62X_OTP_CMD_FG3); > > + if (ret) > > + break; > > + > > + ret = regmap_read(iqs62x->map, IQS62X_OTP_DATA, > > + &val); > > + if (ret) > > + break; > > + > > + hall_cal_index = val & IQS62X_HALL_CAL_MASK; > > + if (!hall_cal_index) { > > + dev_err(&client->dev, > > + "Uncalibrated device\n"); > > + ret = -ENODATA; > > + break; > > + } > > + } > > + > > + if (hall_cal_index > fw_rec->len) { > > + ret = -EINVAL; > > + break; > > + } > > + > > + mask = 0; > > + data = &fw_rec->data + hall_cal_index - 1; > > + len = sizeof(*data); > > + break; > > + > > + case IQS62X_FW_REC_TYPE_MASK: > > + if (fw_rec->len < (sizeof(mask) + sizeof(*data))) { > > + ret = -EINVAL; > > + break; > > + } > > + > > + mask = fw_rec->data; > > + data = &fw_rec->data + sizeof(mask); > > + len = sizeof(*data); > > + break; > > + > > + case IQS62X_FW_REC_TYPE_DATA: > > + mask = 0; > > + data = &fw_rec->data; > > + len = fw_rec->len; > > + break; > > + > > + default: > > + dev_err(&client->dev, > > + "Unrecognized record type: 0x%02X\n", > > + fw_rec->type); > > + ret = -EINVAL; > > + } > > + > > + if (ret) > > + break; > > + > > + fw_blk = devm_kzalloc(&client->dev, > > + struct_size(fw_blk, data, len), > > + GFP_KERNEL); > > + if (!fw_blk) { > > + ret = -ENOMEM; > > + break; > > + } > > + > > + fw_blk->addr = fw_rec->addr; > > + fw_blk->mask = mask; > > + fw_blk->len = len; > > + memcpy(fw_blk->data, data, len); > > + > > + list_add(&fw_blk->list, &iqs62x->fw_blk_head); > > + } > > + > > + release_firmware(fw); > > + > > + return ret; > > +} > > Not sure I'm the best person to be reviewing this. > > Maybe someone from drivers/firmware can help out? I perused MAINTAINERS for a catch-all reviewer of sorts, but nobody stood out because most of drivers/firmware seems to be vendor/application-specific. If you have someone in mind, I would be happy to add them to the v5 review. In no way do I oppose having more eyes on this, but in my humble opinion this driver is no different than any others that call variants of request_firmware to gather register overlays and calibration values. I simply defined the file format in this case instead of it already being defined elsewhere. Regardless of where the definition originated, the focus should be whether iqs62x_fw_parse faithfully adheres to that definition and I think you're in just as good of a position to decide (especially since you've helped review past iterations and have context). (I don't pretend to be an expert in defining specifications, but my arguments are that I've modeled this one after existing in-tree examples, I've made the documentation accessible, and it's relatively simple and low-risk in terms of maintenance). > > > +static irqreturn_t iqs62x_irq(int irq, void *context) > > +{ > > + struct iqs62x_core *iqs62x = context; > > + struct i2c_client *client = iqs62x->client; > > + struct iqs62x_event_data event_data; > > + struct iqs62x_event_desc event_desc; > > + enum iqs62x_event_reg event_reg; > > + unsigned long event_flags = 0; > > + int ret, i, j; > > + u8 event_map[IQS62X_EVENT_SIZE]; > > + > > + /* > > + * The device asserts the RDY output to signal the beginning of a > > + * communication window, which is closed by an I2C stop condition. > > + * As such, all interrupt status is captured in a single read and > > + * broadcast to any interested sub-device drivers. > > + */ > > + ret = regmap_raw_read(iqs62x->map, IQS62X_SYS_FLAGS, event_map, > > + sizeof(event_map)); > > + if (ret) { > > + dev_err(&client->dev, "Failed to read device status: %d\n", > > + ret); > > + return IRQ_NONE; > > + } > > + > > + for (i = 0; i < sizeof(event_map); i++) { > > + event_reg = iqs62x->dev_desc->event_regs[iqs62x->ui_sel][i]; > > + > > + switch (event_reg) { > > + case IQS62X_EVENT_UI_LO: > > + event_data.ui_data = get_unaligned_le16(&event_map[i]); > > + /* fall through */ > > + case IQS62X_EVENT_UI_HI: > > + case IQS62X_EVENT_NONE: > > + continue; > > + > > + case IQS62X_EVENT_ALS: > > + event_data.als_flags = event_map[i]; > > + continue; > > + > > + case IQS62X_EVENT_IR: > > + event_data.ir_flags = event_map[i]; > > + continue; > > + > > + case IQS62X_EVENT_INTER: > > + event_data.interval = event_map[i]; > > + continue; > > + > > + case IQS62X_EVENT_HYST: > > + event_map[i] <<= iqs62x->dev_desc->hyst_shift; > > + /* fall through */ > > + case IQS62X_EVENT_WHEEL: > > + case IQS62X_EVENT_HALL: > > + case IQS62X_EVENT_PROX: > > + case IQS62X_EVENT_SYS: > > + break; > > + } > > + > > + for (j = 0; j < IQS62X_NUM_EVENTS; j++) { > > + event_desc = iqs62x_events[j]; > > + > > + if (event_desc.reg != event_reg) > > + continue; > > + > > + if ((event_map[i] & event_desc.mask) == event_desc.val) > > + event_flags |= BIT(j); > > + } > > + } > > + > > + /* > > + * The device resets itself in response to the I2C master stalling > > + * communication past a fixed timeout. In this case, all registers > > + * are restored and any interested sub-device drivers are notified. > > + */ > > + if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) { > > + dev_err(&client->dev, "Unexpected device reset\n"); > > + > > + ret = iqs62x_dev_init(iqs62x); > > + if (ret) { > > + dev_err(&client->dev, > > + "Failed to re-initialize device: %d\n", ret); > > + return IRQ_NONE; > > + } > > + } > > + > > + ret = blocking_notifier_call_chain(&iqs62x->nh, event_flags, > > + &event_data); > > + if (ret & NOTIFY_STOP_MASK) > > + return IRQ_NONE; > > + > > + /* > > + * Once the communication window is closed, a small delay is added to > > + * ensure the device's RDY output has been deasserted by the time the > > + * interrupt handler returns. > > + */ > > + usleep_range(50, 100); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void iqs62x_fw_cb(const struct firmware *fw, void *context) > > Not keen on these overly abbreviated function names. > > What's 'cb'? 'cb' stands for callback; I picked 'fw_cb' after having seen it in a few other drivers. I agree that it's ambiguous and I'll pick something better. > > > +{ > > + struct iqs62x_core *iqs62x = context; > > + struct i2c_client *client = iqs62x->client; > > + int ret; > > + > > + if (fw) { > > + ret = iqs62x_fw_prs(iqs62x, fw); > > I'm guessing 'prs' is parse, but only by the comment. > > Please expand it to something more meaningful. Sure thing, will do. > > > + if (ret) { > > + dev_err(&client->dev, "Failed to parse firmware: %d\n", > > + ret); > > + goto err_out; > > + } > > + } > > + > > + ret = iqs62x_dev_init(iqs62x); > > + if (ret) { > > + dev_err(&client->dev, "Failed to initialize device: %d\n", ret); > > + goto err_out; > > + } > > + > > + ret = devm_request_threaded_irq(&client->dev, client->irq, > > + NULL, iqs62x_irq, IRQF_ONESHOT, > > + client->name, iqs62x); > > + if (ret) { > > + dev_err(&client->dev, "Failed to request IRQ: %d\n", ret); > > + goto err_out; > > + } > > + > > + ret = devm_mfd_add_devices(&client->dev, -1, > > Please use the defines. Sure thing, will do. > > > + iqs62x->dev_desc->sub_devs, > > + iqs62x->dev_desc->num_sub_devs, > > + NULL, 0, NULL); > > + if (ret) > > + dev_err(&client->dev, "Failed to add devices: %d\n", ret); > > "Failed to add child (or sub-) devices. Sure thing, will do. > > > +err_out: > > + complete_all(&iqs62x->fw_done); > > +} > > + > > +static const struct regmap_config iqs62x_map_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = IQS62X_MAX_REG, > > +}; > > + > > +static int iqs62x_probe(struct i2c_client *client) > > +{ > > + struct iqs62x_core *iqs62x; > > + struct iqs62x_info info; > > + unsigned int val; > > + int ret, i, j; > > + u8 sw_num = 0; > > + const char *fw_name = NULL; > > + > > + iqs62x = devm_kzalloc(&client->dev, sizeof(*iqs62x), GFP_KERNEL); > > + if (!iqs62x) > > + return -ENOMEM; > > + > > + i2c_set_clientdata(client, iqs62x); > > + iqs62x->client = client; > > + > > + BLOCKING_INIT_NOTIFIER_HEAD(&iqs62x->nh); > > + INIT_LIST_HEAD(&iqs62x->fw_blk_head); > > + init_completion(&iqs62x->fw_done); > > + > > + iqs62x->map = devm_regmap_init_i2c(client, &iqs62x_map_config); > > + if (IS_ERR(iqs62x->map)) { > > + ret = PTR_ERR(iqs62x->map); > > + dev_err(&client->dev, "Failed to initialize register map: %d\n", > > + ret); > > + return ret; > > + } > > + > > + ret = regmap_raw_read(iqs62x->map, IQS62X_PROD_NUM, &info, > > + sizeof(info)); > > + if (ret) > > + return ret; > > + > > + /* > > + * The following sequence validates the device's product and software > > + * numbers. It then determines if the device is factory-calibrated by > > + * checking for nonzero values in the device's designated calibration > > + * registers (if applicable). Depending on the device, the absence of > > + * calibration data indicates a reduced feature set or invalid device. > > + * > > + * For devices given in both calibrated and uncalibrated versions, the > > + * calibrated version (e.g. IQS620AT) appears first in the iqs62x_devs > > + * array. The uncalibrated version (e.g. IQS620A) appears next and has > > + * the same product and software numbers, but no calibration registers > > + * are specified. > > + */ > > How long did it take you to supplement words out such that the lines > were all matched up at the ends? ;) I was wondering how long it would take to get caught :) > > > + for (i = 0; i < IQS62X_NUM_DEV; i++) { > > + if (info.prod_num != iqs62x_devs[i].prod_num) > > + continue; > > '\n' Sure thing, will do. > > > + iqs62x->dev_desc = &iqs62x_devs[i]; > > + > > + if (info.sw_num < iqs62x->dev_desc->sw_num) > > + continue; > > '\n' Sure thing, will do. > > > + sw_num = info.sw_num; > > + > > + /* > > + * Read each of the device's designated calibration registers, > > + * if any, and exit from the inner loop early if any are equal > > + * to zero. > > You should mention why zeroed calibration values are a problem. Sure thing, will do. > > > + */ > > + for (j = 0; j < iqs62x->dev_desc->num_cal_regs; j++) { > > + ret = regmap_read(iqs62x->map, > > + iqs62x->dev_desc->cal_regs[j], &val); > > + if (ret) > > + return ret; > > + > > + if (!val) > > + break; > > + } > > + > > + /* > > + * If the number of nonzero values read from the device equals > > + * the number of designated calibration registers (which could > > + * be zero), exit from the outer loop early to signal a device > > + * has been matched. > > This is *what* you are doing, but *why* are you doing it? Sure thing, I'll clarify this comment. > > > + */ > > + if (j == iqs62x->dev_desc->num_cal_regs) > > + break; > > + } > > + > > + if (!iqs62x->dev_desc) { > > + dev_err(&client->dev, "Unrecognized product number: 0x%02X\n", > > + info.prod_num); > > + return -EINVAL; > > + } > > + > > + if (!sw_num) { > > + dev_err(&client->dev, "Unrecognized software number: 0x%02X\n", > > + info.sw_num); > > + return -EINVAL; > > + } > > + > > + if (i == IQS62X_NUM_DEV) { > > + dev_err(&client->dev, "Uncalibrated device\n"); > > + return -ENODATA; > > + } > > + > > + ret = regmap_write(iqs62x->map, IQS62X_SYS_SETTINGS, > > + IQS62X_SYS_SETTINGS_SOFT_RESET); > > + if (ret) > > + return ret; > > '\n' Sure thing, will do (this step may be removed pending discussion in patch [4/7]). > > > + usleep_range(10000, 10100); > > + > > + device_property_read_string(&client->dev, "firmware-name", &fw_name); > > + > > + ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG, > > + fw_name ? : iqs62x->dev_desc->fw_name, > > + &client->dev, GFP_KERNEL, iqs62x, > > + iqs62x_fw_cb); > > + if (ret) > > + dev_err(&client->dev, "Failed to request firmware: %d\n", ret); > > + > > + return ret; > > +} > > + > > +static int iqs62x_remove(struct i2c_client *client) > > +{ > > + struct iqs62x_core *iqs62x = i2c_get_clientdata(client); > > + > > + wait_for_completion(&iqs62x->fw_done); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused iqs62x_suspend(struct device *dev) > > +{ > > + struct iqs62x_core *iqs62x = dev_get_drvdata(dev); > > + int ret; > > + > > + wait_for_completion(&iqs62x->fw_done); > > + > > + /* > > + * As per the data sheet, automatic mode switching must be disabled > > "datasheet" Sure thing, will do. > > > + * before the device is placed in or taken out of halt mode. > > + */ > > + ret = regmap_update_bits(iqs62x->map, IQS62X_PWR_SETTINGS, > > + IQS62X_PWR_SETTINGS_DIS_AUTO, > > + IQS62X_PWR_SETTINGS_DIS_AUTO); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(iqs62x->map, IQS62X_PWR_SETTINGS, > > + IQS62X_PWR_SETTINGS_PWR_MODE_MASK, > > + IQS62X_PWR_SETTINGS_PWR_MODE_HALT); > > +} > > + > > +static int __maybe_unused iqs62x_resume(struct device *dev) > > +{ > > + struct iqs62x_core *iqs62x = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = regmap_update_bits(iqs62x->map, IQS62X_PWR_SETTINGS, > > + IQS62X_PWR_SETTINGS_PWR_MODE_MASK, > > + IQS62X_PWR_SETTINGS_PWR_MODE_NORM); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(iqs62x->map, IQS62X_PWR_SETTINGS, > > + IQS62X_PWR_SETTINGS_DIS_AUTO, 0); > > +} > > + > > +static SIMPLE_DEV_PM_OPS(iqs62x_pm, iqs62x_suspend, iqs62x_resume); > > + > > +static const struct of_device_id iqs62x_of_match[] = { > > + { .compatible = "azoteq,iqs620a" }, > > + { .compatible = "azoteq,iqs621" }, > > + { .compatible = "azoteq,iqs622" }, > > + { .compatible = "azoteq,iqs624" }, > > + { .compatible = "azoteq,iqs625" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, iqs62x_of_match); > > + > > +static struct i2c_driver iqs62x_i2c_driver = { > > + .driver = { > > + .name = "iqs62x", > > + .of_match_table = iqs62x_of_match, > > + .pm = &iqs62x_pm, > > + }, > > + .probe_new = iqs62x_probe, > > + .remove = iqs62x_remove, > > +}; > > +module_i2c_driver(iqs62x_i2c_driver); > > + > > +MODULE_AUTHOR("Jeff LaBundy <jeff@xxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Azoteq IQS620A/621/622/624/625 Multi-Function Sensors"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/mfd/iqs62x-tables.c b/drivers/mfd/iqs62x-tables.c > > Why have you separated this out? > > Prefer to see it as part of the main source file. I was modeling some of the other MFDs that seem to break large static structs and/or register-related items into a separate *tables.c, with *core.c housing the meat of the driver. However, those examples often have well over a thousand lines which isn't the case here. I do think having one source file would make the driver easier to follow, so I'll move everything from *tables.c into *core.c as you suggest. > > > new file mode 100644 > > index 0000000..580f6ac > > --- /dev/null > > +++ b/drivers/mfd/iqs62x-tables.c > > @@ -0,0 +1,438 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Azoteq IQS620A/621/622/624/625 Multi-Function Sensors > > + * > > + * Copyright (C) 2019 Jeff LaBundy <jeff@xxxxxxxxxxx> > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/iqs62x.h> > > + > > +#define IQS620_HALL_FLAGS 0x16 > > +#define IQS620_TEMP_CAL_MULT 0xC2 > > +#define IQS620_TEMP_CAL_DIV 0xC3 > > +#define IQS620_TEMP_CAL_OFFS 0xC4 > > + > > +#define IQS621_HALL_FLAGS 0x19 > > +#define IQS621_ALS_CAL_DIV_LUX 0x82 > > +#define IQS621_ALS_CAL_DIV_IR 0x83 > > + > > +#define IQS622_HALL_FLAGS IQS621_HALL_FLAGS > > + > > +#define IQS624_INTERVAL_NUM 0x18 > > +#define IQS625_INTERVAL_NUM 0x12 > > + > > +static const struct mfd_cell iqs620at_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > Really not keen on names being defined. > > Can you use the proper names please? Sure thing, will do. > > > + .of_compatible = "azoteq,iqs620a-keys", > > + }, > > + { > > + .name = IQS620_DRV_NAME_PWM, > > + .of_compatible = "azoteq,iqs620a-pwm", > > + }, > > + { > > + .name = IQS620_DRV_NAME_TEMP, > > + }, > > +}; > > + > > +static const struct mfd_cell iqs620a_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > + .of_compatible = "azoteq,iqs620a-keys", > > + }, > > + { > > + .name = IQS620_DRV_NAME_PWM, > > + .of_compatible = "azoteq,iqs620a-pwm", > > + }, > > +}; > > + > > +static const struct mfd_cell iqs621_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > + .of_compatible = "azoteq,iqs621-keys", > > + }, > > + { > > + .name = IQS621_DRV_NAME_ALS, > > + }, > > One line please: > > { .name = IQS621_DRV_NAME_ALS, }, Sure thing, will do. > > > +}; > > + > > +static const struct mfd_cell iqs622_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > + .of_compatible = "azoteq,iqs622-keys", > > + }, > > + { > > + .name = IQS621_DRV_NAME_ALS, > > + }, > > As above. Sure thing, will do. > > > +}; > > + > > +static const struct mfd_cell iqs624_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > + .of_compatible = "azoteq,iqs624-keys", > > + }, > > + { > > + .name = IQS624_DRV_NAME_POS, > > + }, > > +}; > > As above. Sure thing, will do. > > > + > > +static const struct mfd_cell iqs625_sub_devs[] = { > > + { > > + .name = IQS62X_DRV_NAME_KEYS, > > + .of_compatible = "azoteq,iqs625-keys", > > + }, > > + { > > + .name = IQS624_DRV_NAME_POS, > > + }, > > As above. Sure thing, will do. > > > +}; > > + > > +static const u8 iqs620at_cal_regs[] = { > > + IQS620_TEMP_CAL_MULT, > > + IQS620_TEMP_CAL_DIV, > > + IQS620_TEMP_CAL_OFFS, > > +}; > > + > > +static const u8 iqs621_cal_regs[] = { > > + IQS621_ALS_CAL_DIV_LUX, > > + IQS621_ALS_CAL_DIV_IR, > > +}; > > + > > +static const enum iqs62x_event_reg iqs620a_event_regs[][IQS62X_EVENT_SIZE] = { > > + [IQS62X_UI_PROX] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_PROX, /* 0x12 */ > > + IQS62X_EVENT_HYST, /* 0x13 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_HALL, /* 0x16 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + }, > > + [IQS62X_UI_SAR1] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_HYST, /* 0x13 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_HALL, /* 0x16 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + }, > > +}; > > + > > +static const enum iqs62x_event_reg iqs621_event_regs[][IQS62X_EVENT_SIZE] = { > > + [IQS62X_UI_PROX] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_PROX, /* 0x12 */ > > + IQS62X_EVENT_HYST, /* 0x13 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_ALS, /* 0x16 */ > > + IQS62X_EVENT_UI_LO, /* 0x17 */ > > + IQS62X_EVENT_UI_HI, /* 0x18 */ > > + IQS62X_EVENT_HALL, /* 0x19 */ > > + }, > > +}; > > + > > +static const enum iqs62x_event_reg iqs622_event_regs[][IQS62X_EVENT_SIZE] = { > > + [IQS62X_UI_PROX] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_PROX, /* 0x12 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_ALS, /* 0x14 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_IR, /* 0x16 */ > > + IQS62X_EVENT_UI_LO, /* 0x17 */ > > + IQS62X_EVENT_UI_HI, /* 0x18 */ > > + IQS62X_EVENT_HALL, /* 0x19 */ > > + }, > > + [IQS62X_UI_SAR1] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_HYST, /* 0x13 */ > > + IQS62X_EVENT_ALS, /* 0x14 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_IR, /* 0x16 */ > > + IQS62X_EVENT_UI_LO, /* 0x17 */ > > + IQS62X_EVENT_UI_HI, /* 0x18 */ > > + IQS62X_EVENT_HALL, /* 0x19 */ > > + }, > > +}; > > + > > +static const enum iqs62x_event_reg iqs624_event_regs[][IQS62X_EVENT_SIZE] = { > > + [IQS62X_UI_PROX] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_PROX, /* 0x12 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_WHEEL, /* 0x14 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_UI_LO, /* 0x16 */ > > + IQS62X_EVENT_UI_HI, /* 0x17 */ > > + IQS62X_EVENT_INTER, /* 0x18 */ > > + IQS62X_EVENT_NONE, > > + }, > > +}; > > + > > +static const enum iqs62x_event_reg iqs625_event_regs[][IQS62X_EVENT_SIZE] = { > > + [IQS62X_UI_PROX] = { > > + IQS62X_EVENT_SYS, /* 0x10 */ > > + IQS62X_EVENT_PROX, /* 0x11 */ > > + IQS62X_EVENT_INTER, /* 0x12 */ > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_NONE, > > + }, > > +}; > > + > > +enum { > > + IQS620AT_DEV, > > + IQS620A_DEV, > > + IQS621_DEV, > > + IQS622_DEV, > > + IQS624_DEV, > > + IQS625_DEV, > > +}; > > + > > +const struct iqs62x_dev_desc iqs62x_devs[IQS62X_NUM_DEV] = { > > + [IQS620AT_DEV] = { > > + .dev_name = "iqs620at", > > + .sub_devs = iqs620at_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs620at_sub_devs), > > + > > + .prod_num = IQS620_PROD_NUM, > > + .sw_num = 0x08, > > + .cal_regs = iqs620at_cal_regs, > > + .num_cal_regs = ARRAY_SIZE(iqs620at_cal_regs), > > + > > + .prox_mask = BIT(0), > > + .sar_mask = BIT(1) | BIT(7), > > + .hall_mask = BIT(2), > > + .hyst_mask = BIT(3), > > + .temp_mask = BIT(4), > > + > > + .hall_flags = IQS620_HALL_FLAGS, > > + > > + .clk_div = 4, > > + .fw_name = "iqs620a.bin", > > + .event_regs = &iqs620a_event_regs[IQS62X_UI_PROX], > > + }, > > + [IQS620A_DEV] = { > > + .dev_name = "iqs620a", > > + .sub_devs = iqs620a_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs620a_sub_devs), > > + > > + .prod_num = IQS620_PROD_NUM, > > + .sw_num = 0x08, > > + > > + .prox_mask = BIT(0), > > + .sar_mask = BIT(1) | BIT(7), > > + .hall_mask = BIT(2), > > + .hyst_mask = BIT(3), > > + .temp_mask = BIT(4), > > + > > + .hall_flags = IQS620_HALL_FLAGS, > > + > > + .clk_div = 4, > > + .fw_name = "iqs620a.bin", > > + .event_regs = &iqs620a_event_regs[IQS62X_UI_PROX], > > + }, > > + [IQS621_DEV] = { > > + .dev_name = "iqs621", > > + .sub_devs = iqs621_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs621_sub_devs), > > + > > + .prod_num = IQS621_PROD_NUM, > > + .sw_num = 0x09, > > + .cal_regs = iqs621_cal_regs, > > + .num_cal_regs = ARRAY_SIZE(iqs621_cal_regs), > > + > > + .prox_mask = BIT(0), > > + .hall_mask = BIT(1), > > + .als_mask = BIT(2), > > + .hyst_mask = BIT(3), > > + .temp_mask = BIT(4), > > + > > + .als_flags = IQS621_ALS_FLAGS, > > + .hall_flags = IQS621_HALL_FLAGS, > > + .hyst_shift = 5, > > + > > + .clk_div = 2, > > + .fw_name = "iqs621.bin", > > + .event_regs = &iqs621_event_regs[IQS62X_UI_PROX], > > + }, > > + [IQS622_DEV] = { > > + .dev_name = "iqs622", > > + .sub_devs = iqs622_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs622_sub_devs), > > + > > + .prod_num = IQS622_PROD_NUM, > > + .sw_num = 0x06, > > + > > + .prox_mask = BIT(0), > > + .sar_mask = BIT(1), > > + .hall_mask = BIT(2), > > + .als_mask = BIT(3), > > + .ir_mask = BIT(4), > > + > > + .als_flags = IQS622_ALS_FLAGS, > > + .hall_flags = IQS622_HALL_FLAGS, > > + > > + .clk_div = 2, > > + .fw_name = "iqs622.bin", > > + .event_regs = &iqs622_event_regs[IQS62X_UI_PROX], > > + }, > > + [IQS624_DEV] = { > > + .dev_name = "iqs624", > > + .sub_devs = iqs624_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs624_sub_devs), > > + > > + .prod_num = IQS624_PROD_NUM, > > + .sw_num = 0x0B, > > + > > + .interval = IQS624_INTERVAL_NUM, > > + .interval_div = 3, > > + > > + .clk_div = 2, > > + .fw_name = "iqs624.bin", > > + .event_regs = &iqs624_event_regs[IQS62X_UI_PROX], > > + }, > > + [IQS625_DEV] = { > > + .dev_name = "iqs625", > > + .sub_devs = iqs625_sub_devs, > > + .num_sub_devs = ARRAY_SIZE(iqs625_sub_devs), > > + > > + .prod_num = IQS625_PROD_NUM, > > + .sw_num = 0x0B, > > + > > + .interval = IQS625_INTERVAL_NUM, > > + .interval_div = 10, > > + > > + .clk_div = 2, > > + .fw_name = "iqs625.bin", > > + .event_regs = &iqs625_event_regs[IQS62X_UI_PROX], > > + }, > > +}; > > +EXPORT_SYMBOL_GPL(iqs62x_devs); > > + > > +const struct iqs62x_event_desc iqs62x_events[IQS62X_NUM_EVENTS] = { > > + [IQS62X_EVENT_PROX_CH0_T] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(4), > > + .val = BIT(4), > > + }, > > + [IQS62X_EVENT_PROX_CH0_P] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(0), > > + .val = BIT(0), > > + }, > > + [IQS62X_EVENT_PROX_CH1_T] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(5), > > + .val = BIT(5), > > + }, > > + [IQS62X_EVENT_PROX_CH1_P] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(1), > > + .val = BIT(1), > > + }, > > + [IQS62X_EVENT_PROX_CH2_T] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(6), > > + .val = BIT(6), > > + }, > > + [IQS62X_EVENT_PROX_CH2_P] = { > > + .reg = IQS62X_EVENT_PROX, > > + .mask = BIT(2), > > + .val = BIT(2), > > + }, > > + [IQS62X_EVENT_HYST_POS_T] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(6) | BIT(7), > > + .val = BIT(6), > > + }, > > + [IQS62X_EVENT_HYST_POS_P] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(5) | BIT(7), > > + .val = BIT(5), > > + }, > > + [IQS62X_EVENT_HYST_NEG_T] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(6) | BIT(7), > > + .val = BIT(6) | BIT(7), > > + }, > > + [IQS62X_EVENT_HYST_NEG_P] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(5) | BIT(7), > > + .val = BIT(5) | BIT(7), > > + }, > > + [IQS62X_EVENT_SAR1_ACT] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(4), > > + .val = BIT(4), > > + }, > > + [IQS62X_EVENT_SAR1_QRD] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(2), > > + .val = BIT(2), > > + }, > > + [IQS62X_EVENT_SAR1_MOVE] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(1), > > + .val = BIT(1), > > + }, > > + [IQS62X_EVENT_SAR1_HALT] = { > > + .reg = IQS62X_EVENT_HYST, > > + .mask = BIT(0), > > + .val = BIT(0), > > + }, > > + [IQS62X_EVENT_WHEEL_UP] = { > > + .reg = IQS62X_EVENT_WHEEL, > > + .mask = BIT(7) | BIT(6), > > + .val = BIT(7), > > + }, > > + [IQS62X_EVENT_WHEEL_DN] = { > > + .reg = IQS62X_EVENT_WHEEL, > > + .mask = BIT(7) | BIT(6), > > + .val = BIT(7) | BIT(6), > > + }, > > + [IQS62X_EVENT_HALL_N_T] = { > > + .reg = IQS62X_EVENT_HALL, > > + .mask = BIT(2) | BIT(0), > > + .val = BIT(2), > > + }, > > + [IQS62X_EVENT_HALL_N_P] = { > > + .reg = IQS62X_EVENT_HALL, > > + .mask = BIT(1) | BIT(0), > > + .val = BIT(1), > > + }, > > + [IQS62X_EVENT_HALL_S_T] = { > > + .reg = IQS62X_EVENT_HALL, > > + .mask = BIT(2) | BIT(0), > > + .val = BIT(2) | BIT(0), > > + }, > > + [IQS62X_EVENT_HALL_S_P] = { > > + .reg = IQS62X_EVENT_HALL, > > + .mask = BIT(1) | BIT(0), > > + .val = BIT(1) | BIT(0), > > + }, > > + [IQS62X_EVENT_SYS_RESET] = { > > + .reg = IQS62X_EVENT_SYS, > > + .mask = BIT(7), > > + .val = BIT(7), > > + }, > > +}; > > +EXPORT_SYMBOL_GPL(iqs62x_events); > > diff --git a/include/linux/mfd/iqs62x.h b/include/linux/mfd/iqs62x.h > > new file mode 100644 > > index 0000000..0dc5997 > > --- /dev/null > > +++ b/include/linux/mfd/iqs62x.h > > @@ -0,0 +1,146 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Azoteq IQS620A/621/622/624/625 Multi-Function Sensors > > + * > > + * Copyright (C) 2019 Jeff LaBundy <jeff@xxxxxxxxxxx> > > + */ > > + > > +#ifndef __LINUX_MFD_IQS62X_H > > +#define __LINUX_MFD_IQS62X_H > > + > > +#define IQS620_PROD_NUM 0x41 > > +#define IQS621_PROD_NUM 0x46 > > +#define IQS622_PROD_NUM 0x42 > > +#define IQS624_PROD_NUM 0x43 > > +#define IQS625_PROD_NUM 0x4E > > + > > +#define IQS621_ALS_FLAGS 0x16 > > +#define IQS622_ALS_FLAGS 0x14 > > + > > +#define IQS624_HALL_UI 0x70 > > +#define IQS624_HALL_UI_WHL_EVENT BIT(4) > > +#define IQS624_HALL_UI_INT_EVENT BIT(3) > > +#define IQS624_HALL_UI_AUTO_CAL BIT(2) > > + > > +#define IQS624_INTERVAL_DIV 0x7D > > + > > +#define IQS620_GLBL_EVENT_MASK 0xD7 > > +#define IQS620_GLBL_EVENT_MASK_PMU BIT(6) > > + > > +#define IQS62X_NUM_DEV 6 > > +#define IQS62X_NUM_KEYS 16 > > +#define IQS62X_NUM_EVENTS (IQS62X_NUM_KEYS + 5) > > + > > +#define IQS62X_EVENT_SIZE 10 > > + > > +#define IQS62X_DRV_NAME_KEYS "iqs62x-keys" > > +#define IQS620_DRV_NAME_TEMP "iqs620at-temp" > > +#define IQS620_DRV_NAME_PWM "iqs620a-pwm" > > +#define IQS621_DRV_NAME_ALS "iqs621-als" > > +#define IQS624_DRV_NAME_POS "iqs624-pos" > > + > > +enum iqs62x_ui_sel { > > + IQS62X_UI_PROX, > > + IQS62X_UI_SAR1, > > +}; > > + > > +enum iqs62x_event_reg { > > + IQS62X_EVENT_NONE, > > + IQS62X_EVENT_SYS, > > + IQS62X_EVENT_PROX, > > + IQS62X_EVENT_HYST, > > + IQS62X_EVENT_HALL, > > + IQS62X_EVENT_ALS, > > + IQS62X_EVENT_IR, > > + IQS62X_EVENT_WHEEL, > > + IQS62X_EVENT_INTER, > > + IQS62X_EVENT_UI_LO, > > + IQS62X_EVENT_UI_HI, > > +}; > > + > > +enum iqs62x_event_flag { > > + /* keys */ > > + IQS62X_EVENT_PROX_CH0_T, > > + IQS62X_EVENT_PROX_CH0_P, > > + IQS62X_EVENT_PROX_CH1_T, > > + IQS62X_EVENT_PROX_CH1_P, > > + IQS62X_EVENT_PROX_CH2_T, > > + IQS62X_EVENT_PROX_CH2_P, > > + IQS62X_EVENT_HYST_POS_T, > > + IQS62X_EVENT_HYST_POS_P, > > + IQS62X_EVENT_HYST_NEG_T, > > + IQS62X_EVENT_HYST_NEG_P, > > + IQS62X_EVENT_SAR1_ACT, > > + IQS62X_EVENT_SAR1_QRD, > > + IQS62X_EVENT_SAR1_MOVE, > > + IQS62X_EVENT_SAR1_HALT, > > + IQS62X_EVENT_WHEEL_UP, > > + IQS62X_EVENT_WHEEL_DN, > > + > > + /* switches */ > > + IQS62X_EVENT_HALL_N_T, > > + IQS62X_EVENT_HALL_N_P, > > + IQS62X_EVENT_HALL_S_T, > > + IQS62X_EVENT_HALL_S_P, > > + > > + /* everything else */ > > + IQS62X_EVENT_SYS_RESET, > > +}; > > + > > +struct iqs62x_event_data { > > + u16 ui_data; > > + u8 als_flags; > > + u8 ir_flags; > > + u8 interval; > > +}; > > + > > +struct iqs62x_event_desc { > > + enum iqs62x_event_reg reg; > > + u8 mask; > > + u8 val; > > +}; > > + > > +struct iqs62x_dev_desc { > > + const char *dev_name; > > + const struct mfd_cell *sub_devs; > > + int num_sub_devs; > > + > > + u8 prod_num; > > + u8 sw_num; > > + const u8 *cal_regs; > > + int num_cal_regs; > > + > > + u8 prox_mask; > > + u8 sar_mask; > > + u8 hall_mask; > > + u8 hyst_mask; > > + u8 temp_mask; > > + u8 als_mask; > > + u8 ir_mask; > > + > > + u8 als_flags; > > + u8 hall_flags; > > + u8 hyst_shift; > > + > > + u8 interval; > > + u8 interval_div; > > + > > + u8 clk_div; > > + const char *fw_name; > > + const enum iqs62x_event_reg (*event_regs)[IQS62X_EVENT_SIZE]; > > +}; > > + > > +struct iqs62x_core { > > + const struct iqs62x_dev_desc *dev_desc; > > + struct i2c_client *client; > > + struct regmap *map; > > Prefer 'regmap', it's less ambiguous. Sure thing, will do. > > > + struct blocking_notifier_head nh; > > + struct list_head fw_blk_head; > > + struct completion fw_done; > > + enum iqs62x_ui_sel ui_sel; > > +}; > > + > > +extern const struct iqs62x_dev_desc iqs62x_devs[IQS62X_NUM_DEV]; > > +extern const struct iqs62x_event_desc iqs62x_events[IQS62X_NUM_EVENTS]; > > + > > +#endif /* __LINUX_MFD_IQS62X_H */ > > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog Kind regards, Jeff LaBundy
