On Sun, 20 Oct 2019, Jeff LaBundy wrote: > This patch adds support for core functions common to all six-channel > members of the Azoteq ProxFusion family of sensor devices. > > Signed-off-by: Jeff LaBundy <jeff@xxxxxxxxxxx> > --- > drivers/mfd/Kconfig | 13 + > drivers/mfd/Makefile | 2 + > drivers/mfd/iqs62x-core.c | 638 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/mfd/iqs62x-tables.c | 424 +++++++++++++++++++++++++++++ > include/linux/mfd/iqs62x.h | 148 ++++++++++ > 5 files changed, 1225 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 ae24d3e..df391f7 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 support for six-channel members of > + the Azoteq ProxFusion family of sensor devices. 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 c1067ea..23dd71c6 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -256,3 +256,5 @@ obj-$(CONFIG_MFD_ROHM_BD70528) += rohm-bd70528.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > +iqs62x-objs := iqs62x-core.o iqs62x-tables.o > +obj-$(CONFIG_MFD_IQS62X) += iqs62x.o > diff --git a/drivers/mfd/iqs62x-core.c b/drivers/mfd/iqs62x-core.c > new file mode 100644 > index 0000000..e2200c8 > --- /dev/null > +++ b/drivers/mfd/iqs62x-core.c > @@ -0,0 +1,638 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Azoteq IQS620A/621/622/624/625 ProxFusion Sensor Family > + * > + * Copyright (C) 2019 Needs a company or person's name. > + * Author: 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. This troubles me somewhat. So here we take a C header file which is the output of a vendor supplied configuration tool, convert it to a bespoke file format using a Python script authored by yourself, which we masquerade as Linux firmware in order to set-up the hardware. Is that correct? What is preventing a very naughty person from providing their own register map (firmware) in order to read/write unsuitable registers from kernel context for their own gains; simply by swapping out a file contained in userspace? It would probably be a better idea to compile the register definitions with the kernel/module to be safe. You can use Device Tree for run-time configuration changes. > + * Link to data sheets and GUIs: https://www.azoteq.com/products/proxfusion/ > + * > + * Link to conversion tool: https://github.com/jlabundy/iqs62x-h2bin.git This is unlikely to stand the test of time. Probably best to just list the name of the tool in the description above. > + */ > + > +#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> [...] > +static int iqs62x_fw_parse(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 hall_cal_index = 0; > + size_t pos = 0; > + int error = 0; > + u8 mask, len; > + u8 *data; > + > + while (pos < fw->size) { > + if (pos + sizeof(*fw_rec) > fw->size) { > + error = -EINVAL; > + break; > + } > + fw_rec = (struct iqs62x_fw_rec *)(fw->data + pos); > + pos += sizeof(*fw_rec); > + > + if (pos + fw_rec->len - 1 > fw->size) { > + error = -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); > + error = -EINVAL; > + break; > + > + case IQS62X_FW_REC_TYPE_HALL: > + if (!hall_cal_index) { > + error = regmap_write(iqs62x->map, > + IQS62X_OTP_CMD, > + IQS62X_OTP_CMD_FG3); > + if (error) > + break; > + > + error = regmap_read(iqs62x->map, > + IQS62X_OTP_DATA, > + &hall_cal_index); > + if (error) > + break; > + > + hall_cal_index &= IQS62X_HALL_CAL_MASK; > + if (!hall_cal_index) { > + dev_err(&client->dev, > + "Uncalibrated device\n"); > + error = -ENODATA; > + break; > + } > + } > + > + if (hall_cal_index > fw_rec->len) { > + error = -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))) { > + error = -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); > + error = -EINVAL; > + } > + > + if (error) > + break; > + > + fw_blk = devm_kzalloc(&client->dev, > + struct_size(fw_blk, data, len), > + GFP_KERNEL); > + if (!fw_blk) { > + error = -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 error; > +} > + > +static irqreturn_t iqs62x_irq(int irq, void *context) > +{ > + struct iqs62x_core *iqs62x = context; > + struct iqs62x_event_data event_data; > + struct iqs62x_event_desc event_desc; > + enum iqs62x_event_reg event_reg; > + unsigned long event_flags = 0; > + int error, 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. > + */ > + error = regmap_raw_read(iqs62x->map, IQS62X_SYS_FLAGS, > + event_map, sizeof(event_map)); > + if (error) > + 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: > + case IQS62X_EVENT_GLBL: > + continue; > + > + case IQS62X_EVENT_TEMP: > + event_data.temp_flags = event_map[i]; > + 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 beyond a 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(&iqs62x->client->dev, "Unexpected device reset\n"); > + > + error = iqs62x_dev_init(iqs62x); Is it safe to re-initialise the entire device in IRQ context? > + if (error) { > + dev_err(&iqs62x->client->dev, > + "Failed to re-initialize device: %d\n", error); > + return IRQ_NONE; > + } > + } > + > + error = blocking_notifier_call_chain(&iqs62x->nh, event_flags, > + &event_data); > + if (error & 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 int iqs62x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct iqs62x_core *iqs62x; > + struct iqs62x_info info; > + unsigned int val; > + int error, i, j; Nit: It's more common to use 'ret' or 'err' - my preference is 'ret'. > + const char *fw_file = 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)) { > + error = PTR_ERR(iqs62x->map); > + dev_err(&client->dev, "Failed to initialize register map: %d\n", > + error); > + return error; > + } > + > + error = regmap_raw_read(iqs62x->map, IQS62X_PROD_NUM, &info, > + sizeof(info)); > + if (error) > + return error; > + > + for (i = 0; i < IQS62X_NUM_DEV; i++) { > + if (info.prod_num == iqs62x_devs[i].prod_num) > + iqs62x->dev_desc = &iqs62x_devs[i]; > + else > + continue; Reads better without the else: if (info.prod_num != iqs62x_devs[i].prod_num) continue; > + if (info.sw_num >= iqs62x->dev_desc->sw_num) > + iqs62x->sw_num = info.sw_num; > + else > + continue; Same as above. Do you still need to be in this loop at this point? > + for (j = 0; j < iqs62x->dev_desc->num_cal_regs; j++) { What are you doing here? Please provide a comment. > + error = regmap_read(iqs62x->map, > + iqs62x->dev_desc->cal_regs[j], > + &val); > + if (error) > + return error; > + > + if (!val) > + break; > + } > + > + if (j == iqs62x->dev_desc->num_cal_regs) > + break; Is there a reason not to break here? If the product number matched once, can it match for a second time? > + } > + > + if (!iqs62x->dev_desc) { > + dev_err(&client->dev, "Unrecognized product number: 0x%02X\n", > + info.prod_num); > + return -EINVAL; > + } > + > + if (!iqs62x->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; > + } > + > + error = regmap_write(iqs62x->map, IQS62X_SYS_SETTINGS, > + IQS62X_SYS_SETTINGS_SOFT_RESET); > + if (error) > + return error; > + usleep_range(10000, 10100); > + > + device_property_read_string(&client->dev, "linux,fw-file", &fw_file); > + > + error = request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG, > + fw_file ? : iqs62x->dev_desc->fw_file, > + &client->dev, GFP_KERNEL, iqs62x, > + iqs62x_fw_load); > + if (error) > + dev_err(&client->dev, "Failed to request firmware: %d\n", > + error); > + > + return error; > +} > + > +static int iqs62x_remove(struct i2c_client *client) > +{ > + struct iqs62x_core *iqs62x = i2c_get_clientdata(client); > + > + wait_for_completion(&iqs62x->fw_done); > + > + return 0; > +} Please move the suspend/resume calls down to here. > +static const struct i2c_device_id iqs62x_id[] = { > + { "iqs620a", 0 }, > + { "iqs621", 1 }, > + { "iqs622", 2 }, > + { "iqs624", 3 }, > + { "iqs625", 4 }, Better to define these. In fact, do you even use the .ids? If not, you can use the new I2C probe function and drop this table. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, iqs62x_id); > + > +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, > + }, > + .id_table = iqs62x_id, > + .probe = 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 ProxFusion Sensor Family"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/mfd/iqs62x-tables.c b/drivers/mfd/iqs62x-tables.c > new file mode 100644 > index 0000000..12300b7 > --- /dev/null > +++ b/drivers/mfd/iqs62x-tables.c > @@ -0,0 +1,424 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Azoteq IQS620A/621/622/624/625 ProxFusion Sensor Family > + * > + * Copyright (C) 2019 > + * Author: Jeff LaBundy <jeff@xxxxxxxxxxx> > + */ > + > +#include <linux/kernel.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/iqs62x.h> > + > +static const struct mfd_cell iqs620at_sub_devs[] = { > + { > + .name = IQS62X_DRV_NAME_KEYS, > + .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, > + }, > +}; > + > +static const struct mfd_cell iqs622_sub_devs[] = { > + { > + .name = IQS62X_DRV_NAME_KEYS, > + .of_compatible = "azoteq,iqs622-keys", > + }, > + { > + .name = IQS622_DRV_NAME_PROX, > + .of_compatible = "azoteq,iqs622-prox", > + }, > +}; > + > +static const struct mfd_cell iqs624_sub_devs[] = { > + { > + .name = IQS62X_DRV_NAME_KEYS, > + .of_compatible = "azoteq,iqs624-keys", > + }, > + { > + .name = IQS624_DRV_NAME_POS, > + }, > +}; > + > +static const struct mfd_cell iqs625_sub_devs[] = { > + { > + .name = IQS62X_DRV_NAME_KEYS, > + .of_compatible = "azoteq,iqs625-keys", > + }, > + { > + .name = IQS624_DRV_NAME_POS, > + }, > +}; These should be moved into the core driver. > +static const u8 iqs620at_cal_regs[] = { 0xC2, 0xC3, 0xC4, }; > +static const u8 iqs621_cal_regs[] = { 0x82, 0x83, }; What do these mean? Probably best of do define them, so people can read them. [...] -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
