Re: [PATCH 2/8] mfd: Add support for Azoteq IQS620A/621/622/624/625

Lee Jones
2019-10-31T13:44:17Z

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@labundy.com>
> ---
> 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@labundy.com>
> + *
> + * 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@labundy.com>");
> +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@labundy.com>
> + */
> +
> +#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