Hi Jonathan,
On Sat, 13 Jul 2024 15:27:00 +0100, jic23@xxxxxxxxxx wrote:
>On Fri, 12 Jul 2024 11:32:00 +0000
>wangshuaijie@xxxxxxxxxx wrote:
>
>> From: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>>
>> 1. Modify the structure of the driver.
>> 2. Change the style of the driver's comments.
>> 3. Remove unnecessary log printing.
>> 4. Modify the function used for memory allocation.
>> 5. Modify the driver registration process.
>> 6. Remove the functionality related to updating firmware.
>> 7. Change the input subsystem in the driver to the iio subsystem.
>> 8. Modify the usage of the interrupt pin.
>>
>> Signed-off-by: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>
>As others have observed this is a change log not a patch description so belongs
>below the ---
>Also, should be a patch description.
>
>The patch is very large and hard to review as a result.
>Break it up in to parts. Common infrastructure, then add support
>for one part then for the next one etc. Given size of this, I'd start
>with a submission supporting only one part. Don't abstract anything that
>isn't needed for that part initially. That abstraction should occur
>in a separate series that adds the parts where things differ etc.
>
>That way we get bite sized parts to review. I'll take a quick look but given current status
>this probably won't be a thorough review.
>
>Superficial feedback is this seems like a very complex driver for a not
>very complex of devices. Please look to drop any flexibility in the driver
>that isn't used etc.
>
>Anyhow, I'm out of time, so the feedback is patch at best and I've not cropped
>the reply down.
>
>For next version aim for no patch > 1000 lines and typically much smaller than
>that and ask us to review a subset of what you have here. Build it up in several
>goes. The feedback on that first subset will help you fix up later ones to be
>closer to what we are looking at.
>
>Jonathan
>
Thank you very much for your valuable feedback. The driver for this proximity
sensor is indeed overly complex, and the software architecture is not well
designed.
In the next version of the patch, I will abandon the previous architecture
and focus only on supporting the aw9610x series. This will make the code more
concise and easier to read. In the long run, when designing for compatibility,
I will study the designs of other drivers to make it more compliant with
Linux kernel specifications.
>
>> ---
>> drivers/iio/proximity/Kconfig | 10 +
>> drivers/iio/proximity/Makefile | 2 +
>> drivers/iio/proximity/aw9610x.c | 1150 ++++++++++
>> drivers/iio/proximity/aw963xx.c | 1371 ++++++++++++
>> drivers/iio/proximity/aw_sar.c | 1850 +++++++++++++++++
>> drivers/iio/proximity/aw_sar.h | 23 +
>> drivers/iio/proximity/aw_sar_comm_interface.c | 550 +++++
>> drivers/iio/proximity/aw_sar_comm_interface.h | 172 ++
>> drivers/iio/proximity/aw_sar_type.h | 371 ++++
>> 9 files changed, 5499 insertions(+)
>> create mode 100644 drivers/iio/proximity/aw9610x.c
>> create mode 100644 drivers/iio/proximity/aw963xx.c
>> create mode 100644 drivers/iio/proximity/aw_sar.c
>> create mode 100644 drivers/iio/proximity/aw_sar.h
>> create mode 100644 drivers/iio/proximity/aw_sar_comm_interface.c
>> create mode 100644 drivers/iio/proximity/aw_sar_comm_interface.h
>> create mode 100644 drivers/iio/proximity/aw_sar_type.h
>>
>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
>> index 2ca3b0bc5eba..a60d3dc955b3 100644
>> --- a/drivers/iio/proximity/Kconfig
>> +++ b/drivers/iio/proximity/Kconfig
>> @@ -219,4 +219,14 @@ config VL53L0X_I2C
>> To compile this driver as a module, choose M here: the
>> module will be called vl53l0x-i2c.
>>
>> +config AWINIC_SAR
>> + tristate "Awinic AW96XXX proximity sensor"
>> + depends on I2C
>> + help
>> + Say Y here to build a driver for Awinic's AW96XXX capacitive
>> + proximity sensor.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called awinic_sar.
>> +
>> endmenu
>> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
>> index f36598380446..d4bd9edd8362 100644
>> --- a/drivers/iio/proximity/Makefile
>> +++ b/drivers/iio/proximity/Makefile
>> @@ -21,4 +21,6 @@ obj-$(CONFIG_SX_COMMON) += sx_common.o
>> obj-$(CONFIG_SX9500) += sx9500.o
>> obj-$(CONFIG_VCNL3020) += vcnl3020.o
>> obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
>> +obj-$(CONFIG_AWINIC_SAR) += awinic_sar.o
>> +awinic_sar-objs := aw_sar_comm_interface.o aw_sar.o aw9610x.o aw963xx.o
>>
>> diff --git a/drivers/iio/proximity/aw9610x.c b/drivers/iio/proximity/aw9610x.c
>> new file mode 100644
>> index 000000000000..15e53d55d2a1
>> --- /dev/null
>> +++ b/drivers/iio/proximity/aw9610x.c
>> @@ -0,0 +1,1150 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AWINIC sar sensor driver (aw9610x)
>> + *
>> + * Author: Shuaijie Wang<wangshuaijie@xxxxxxxxxx>
>> + *
>> + * Copyright (c) 2024 awinic Technology CO., LTD
>> + */
>> +#include "aw_sar.h"
>> +
>> +#define AW9610X_I2C_NAME "aw9610x_sar"
>> +#define AW9610X_CHANNEL_MAX (5)
>
>No brackets around numeric values. They add nothing.
>
The patch for v4 will fix these issues.
>> +
>> +#define AFE_BASE_ADDR (0x0000)
>
>
>> +
>> +enum aw9610x_sar_vers {
>> + AW9610X = 2,
>> + AW9610XA = 6,
>> + AW9610XB = 0xa,
>> +};
>> +
>> +enum aw9610x_operation_mode {
>> + AW9610X_ACTIVE_MODE = 1,
>> + AW9610X_SLEEP_MODE,
>> + AW9610X_DEEPSLEEP_MODE,
>> + AW9610XB_DEEPSLEEP_MODE,
>> +};
>> +
>> +enum aw9610x_spereg_addr_offset {
>> + AW_CL1SPE_CALI_OS = 20,
>> + AW_CL1SPE_DEAL_OS = 60,
>> + AW_CL2SPE_CALI_OS = 4,
>> + AW_CL2SPE_DEAL_OS = 4,
>> +};
>> +
>> +enum aw9610x_function_flag {
>> + AW9610X_FUNC_OFF,
>> + AW9610X_FUNC_ON,
>> +};
>> +
>> +enum aw9610x_irq_trigger_position {
>> + AW9610X_FAR,
>> + AW9610X_TRIGGER_TH0,
>> + AW9610X_TRIGGER_TH1 = 0x03,
>> + AW9610X_TRIGGER_TH2 = 0x07,
>> + AW9610X_TRIGGER_TH3 = 0x0f,
>> +};
>> +
>> +struct aw_i2c_package {
>> + unsigned char addr_bytes;
>> + unsigned char data_bytes;
>> + unsigned char reg_num;
>> + unsigned char init_addr[4];
>> + unsigned char *p_reg_data;
>> +};
>> +
>> +struct aw9610x {
>> + unsigned char vers;
>> + unsigned char channel;
>> + unsigned int irq_status;
>> + unsigned char chip_name[9];
>> + unsigned char chip_type[9];
>> + bool satu_release;
>> + struct aw_i2c_package aw_i2c_package;
>> + unsigned char satu_flag[6];
>> + unsigned int satu_data[6];
>> + unsigned int last_blfilta[AW9610X_CHANNEL_MAX];
>> + unsigned int last_irq_en;
>> +};
>> +
>> +struct aw_reg_data {
>> + unsigned char rw;
>> + unsigned short reg;
>
>> +};
>> +#define REG_NONE_ACCESS (0)
>> +#define REG_RD_ACCESS (1 << 0)
>> +#define REG_WR_ACCESS (1 << 1)
>> +
>
>> +
>> +static struct aw_sar *g_aw_sar;
>> +
>> +static int aw9610x_baseline_filter(struct aw_sar *p_sar)
>> +{
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned int status0;
>> + unsigned int status1;
>
>u32 for 32 bit register to make it explicit that they are 32 bits.
>
The patch for v4 will fix these issues.
>> + unsigned char i;
>> +
>> + aw_sar_i2c_read(p_sar->i2c, REG_STAT1, &status1);
>Check return value of all reads.
>
The patch for v4 will fix these issues.
>> + aw_sar_i2c_read(p_sar->i2c, REG_STAT0, &status0);
>> +
>> + for (i = 0; i < AW9610X_CHANNEL_MAX; i++) {
>> + if (((status1 >> i) & 0x01) == 1) {
>> + if (aw9610x->satu_flag[i] == 0) {
>> + aw_sar_i2c_read(p_sar->i2c, REG_BLFILT_CH0 + i * AW_CL1SPE_DEAL_OS,
>> + &aw9610x->satu_data[i]);
>> + aw_sar_i2c_write(p_sar->i2c, REG_BLFILT_CH0 + i * AW_CL1SPE_DEAL_OS,
>> + ((aw9610x->satu_data[i] | 0x1fc) & 0x3fffffff));
>
>Masks should be associated with defines. I have no idea what this code
>is doing so a comment would be good. It looks to be reading current state
>into a satu_data[i] then writing back something different but not updating
>local state. That needs some documentation.
>
>
The v4 version patch will delete related code.
>> + aw9610x->satu_flag[i] = 1;
>> + }
>> + } else if (((status1 >> i) & 0x01) == 0) {
>Given a bit can only be 0 or 1 and previous branch was for 1.
> } else {
>
>> + if (aw9610x->satu_flag[i] == 1) {
>> + if (((status0 >> (i + 24)) & 0x01) == 0) {
>> + aw_sar_i2c_write(p_sar->i2c,
>> + REG_BLFILT_CH0 + i * AW_CL1SPE_DEAL_OS,
>> + aw9610x->satu_data[i]);
>> + aw9610x->satu_flag[i] = 0;
>> + }
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void aw9610x_saturat_release_handle(struct aw_sar *p_sar)
>> +{
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned int satu_irq;
>> + unsigned int status0;
>> + unsigned char i;
>> +
>> + satu_irq = (aw9610x->irq_status >> 7) & 0x01;
>> + if (satu_irq == 1) {
>> + aw9610x_baseline_filter(p_sar);
>> + } else {
>> + aw_sar_i2c_read(p_sar->i2c, REG_STAT0, &status0);
>> + for (i = 0; i < AW9610X_CHANNEL_MAX; i++) {
>> + if (aw9610x->satu_flag[i] == 1) {
>
>If it's a flag, make it boolean.
>
The v4 version patch will delete related code.
>> + if (((status0 >> (i + 24)) & 0x01) == 0) {
>> + aw_sar_i2c_write(p_sar->i2c,
>> + REG_BLFILT_CH0 + i * AW_CL1SPE_DEAL_OS,
>> + aw9610x->satu_data[i]);
>> + aw9610x->satu_flag[i] = 0;
>> + }
>> + }
>> + }
>> + }
>> +}
>> +
>> +static void aw9610x_irq_handle(struct aw_sar *p_sar)
>> +{
>> + unsigned int curr_status_val;
>> + unsigned int curr_status;
>> + unsigned char i;
>> +
>> + aw_sar_i2c_read(p_sar->i2c, REG_STAT0, &curr_status_val);
>> + if (!p_sar->channels_arr) {
>> + dev_err(p_sar->dev, "p_sar->channels_arr err!!!");
>> + return;
>> + }
>> +
>> + for (i = 0; i < AW9610X_CHANNEL_MAX; i++) {
>> + curr_status =
>> + (((unsigned char)(curr_status_val >> (24 + i)) & 0x1))
>WHy is the cast needed?
>> +#ifdef AW_INPUT_TRIGGER_TH1
>
>No ifdefs like this should be seen in a submitted driver.
>
The v4 version patch will delete related code.
>> + | (((unsigned char)(curr_status_val >> (16 + i)) & 0x1) << 1)
>> +#endif
>> +#ifdef AW_INPUT_TRIGGER_TH2
>> + | (((unsigned char)(curr_status_val >> (8 + i)) & 0x1) << 2)
>> +#endif
>> +#ifdef AW_INPUT_TRIGGER_TH3
>> + | (((unsigned char)(curr_status_val >> (i)) & 0x1) << 3)
>> +#endif
>> + ;
>> +
>> + if (p_sar->channels_arr[i].used == AW_FALSE)
>
>Use a boolean and don't invent your own idea of true adn flase.
>
The patch for v4 will fix these issues.
>
>> + continue;
>> +
>> + if (p_sar->channels_arr[i].last_channel_info == curr_status)
>> + continue;
>> +
>> + switch (curr_status) {
>> + case AW9610X_FAR:
>> + iio_push_event(p_sar->aw_iio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i,
>> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
>> + iio_get_time_ns(p_sar->aw_iio_dev));
>> + break;
>> + case AW9610X_TRIGGER_TH0:
>> +#ifdef AW_INPUT_TRIGGER_TH1
>> + case AW9610X_TRIGGER_TH1:
>> +#endif
>> +#ifdef AW_INPUT_TRIGGER_TH2
>> + case AW9610X_TRIGGER_TH2:
>> +#endif
>> +#ifdef AW_INPUT_TRIGGER_TH3
>> + case AW9610X_TRIGGER_TH3:
>> +#endif
>> + iio_push_event(p_sar->aw_iio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i,
>> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING),
>> + iio_get_time_ns(p_sar->aw_iio_dev));
>> + break;
>> + default:
>> + dev_err(p_sar->dev, "error abs distance");
>> + return;
>> + }
>> + p_sar->channels_arr[i].last_channel_info = curr_status;
>> + }
>> +}
>> +
>> +static void aw9610x_version_aw9610x_private(struct aw_sar *p_sar)
>
>For an unusual bit of code like this, add a comment on what is goig on.
>
The v4 version patch will delete related code.
>> +{
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> +
>> + if (aw9610x->satu_release == AW9610X_FUNC_ON)
>> + aw9610x_saturat_release_handle(p_sar);
>> +}
>> +
>> +static void aw9610x_irq_handle_func(unsigned int irq_status, void *data)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> +
>> + switch (aw9610x->vers) {
>> + case AW9610X:
>> + aw9610x_version_aw9610x_private(p_sar);
>> + break;
>> + case AW9610XA:
>If it's the same as default, rely on default.
>
The v4 version patch will delete related code.
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + aw9610x_irq_handle(p_sar);
>> +}
>> +
>> +int aw9610x_check_chipid(void *data)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + unsigned int reg_val;
>> + int ret;
>> +
>> + if (!p_sar)
>> + return -EINVAL;
>> +
>> + ret = aw_sar_i2c_read(p_sar->i2c, REG_CHIPID, ®_val);
>> + if (ret < 0) {
>> + dev_err(p_sar->dev, "read CHIP ID failed: %d", ret);
>> + return ret;
>> + }
>> + reg_val = reg_val >> 16;
>
>FIELD_GET() with appropriately defined mask.
>
The patch for v4 will fix these issues.
>> +
>> + if (reg_val != AW9610X_CHIP_ID) {
>> + dev_err(p_sar->dev, "unsupport dev, chipid is (0x%04x)", reg_val);
>
>
>To allow use of fallback compatibles in DT we normally accept chipid missmatches.
>So at most dev_info and carry on anyway.
>
Sorry, if the chipid does not match, the driver is likely to encounter issues
during operation. We will not consider compatibility with more devices for
the time being.
>> + return -EINVAL;
>> + }
>> + memcpy(p_sar->chip_name, "AW9610X", 8);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(aw9610x_check_chipid, AWINIC_PROX);
>> +
>> +static const struct aw_sar_check_chipid_t g_aw9610x_check_chipid = {
>> + .p_check_chipid_fn = aw9610x_check_chipid,
>> +};
>> +
>> +static ssize_t aw9610x_operation_mode_get(void *data, char *buf)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + ssize_t len = 0;
>> +
>> + if (p_sar->last_mode == AW9610X_ACTIVE_MODE)
>> + len += snprintf(buf + len, PAGE_SIZE - len, "operation mode: Active\n");
> return snprintf()
>
> if ()
> return sprintf;
>etc
>
>However this sort of custom ABI needs docs in Documenation/ABI/testing/sysfs-bus-iio*
>and we are reluctant to accept it because it's the sort of thing userspace has no idea
>how to use. These sleep / deepsleep are marketing terms. Much better the driver uses
>runtime_pm or similar to work out what state it should be in dependent on how it is
>being used etc.
>
In the next version of the patch, I will remove unnecessary interfaces.
>> + else if (p_sar->last_mode == AW9610X_SLEEP_MODE)
>> + len += snprintf(buf + len, PAGE_SIZE - len, "operation mode: Sleep\n");
>> + else if ((p_sar->last_mode == AW9610X_DEEPSLEEP_MODE) && (aw9610x->vers == AW9610XA))
>> + len += snprintf(buf + len, PAGE_SIZE - len, "operation mode: DeepSleep\n");
>> + else
>> + len += snprintf(buf + len, PAGE_SIZE - len, "operation mode: Unconfirmed\n");
>> +
>> + return len;
>> +}
>> +
>> +static void aw9610x_chip_info_get(void *data, char *buf, ssize_t *p_len)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned int reg_data;
>> +
>I can't actually work out where this ends up, but as a general rule formatted data
>doesn't make sense. If this needs to be exposed. One value per file si the way
>to go - not a block of text.
>
The v4 version patch will delete related code.
>> + *p_len += snprintf(buf + *p_len, PAGE_SIZE - *p_len,
>> + "sar%u\n", p_sar->dts_info.sar_num);
>> + *p_len += snprintf(buf + *p_len, PAGE_SIZE - *p_len, "The driver supports UI\n");
>> +
>> + aw_sar_i2c_read(p_sar->i2c, REG_CHIPID, ®_data);
>> + *p_len += snprintf(buf + *p_len, PAGE_SIZE - *p_len, "chipid is 0x%08x\n", reg_data);
>> +
>> + aw_sar_i2c_read(p_sar->i2c, REG_IRQEN, ®_data);
>> + *p_len += snprintf(buf + *p_len, PAGE_SIZE - *p_len, "REG_HOSTIRQEN is 0x%08x\n", reg_data);
>> +
>> + *p_len += snprintf(buf + *p_len, PAGE_SIZE - *p_len,
>> + "chip_name:%s bin_prase_chip_name:%s\n",
>> + aw9610x->chip_name, aw9610x->chip_type);
>> +}
>> +
>> +static const struct aw_sar_get_chip_info_t g_aw9610x_get_chip_info = {
>> + .p_get_chip_info_node_fn = aw9610x_chip_info_get,
>> +};
>> +
>> +static void aw9610x_reg_version_comp(struct aw_sar *p_sar, struct aw_bin *aw_bin)
>> +{
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned int blfilt1_data;
>> + unsigned int blfilt1_tmp;
>> + unsigned char i;
>> +
>> + if ((aw9610x->chip_name[7] == 'A') &&
>
>If there is somthing that is chip specific encode it properly as a
>field in the chip specific structure, not based on chip_name.
>
In the next version of the patch, I will use the chip version for judgment.
>> + (aw_bin->header_info[0].chip_type[7] == '\0')) {
>> + for (i = 0; i < 6; i++) {
>> + aw_sar_i2c_read(p_sar->i2c, REG_BLFILT_CH0 + (0x3c * i), &blfilt1_data);
>> + blfilt1_tmp = (blfilt1_data >> 25) & 0x1;
>FIELD_GET() and appropriately defined mask.
>
The patch for v4 will fix these issues.
>> + if (blfilt1_tmp == 1)
>> + aw_sar_i2c_write_bits(p_sar->i2c, REG_BLRSTRNG_CH0 + (0x3c * i),
>> + ~(0x3f), 1 << i);
>> + }
>> + }
>> +}
>> +
>> +static int aw9610x_load_reg_bin(struct aw_bin *aw_bin, void *load_bin_para)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + int ret;
>> +
>> + snprintf(aw9610x->chip_type, sizeof(aw9610x->chip_type), "%s",
>> + aw_bin->header_info[0].chip_type);
>> + ret = strncmp(aw9610x->chip_name, aw_bin->header_info[0].chip_type,
>> + sizeof(aw_bin->header_info[0].chip_type));
>> + if (ret != 0)
>> + dev_err(p_sar->dev, "load_binname(%s) incompatible with chip type(%s)",
>> + p_sar->chip_name, aw_bin->header_info[0].chip_type);
>Why carry on? It failed, return the error without the next bit.
>
The v4 version patch will delete related code.
>> +
>> + p_sar->load_bin.bin_data_ver = aw_bin->header_info[0].bin_data_ver;
>> +
>> + ret = aw_sar_load_reg(aw_bin, p_sar->i2c);
>> + aw9610x_reg_version_comp(p_sar, aw_bin);
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t aw9610x_get_self_cap_offset(void *data, char *buf)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + unsigned char temp_data[20] = { 0 };
>> + unsigned int coff_data_int;
>> + unsigned int coff_data_dec;
>> + unsigned int coff_data;
>> + unsigned int reg_val;
>> + ssize_t len = 0;
>> + unsigned char i;
>> +
>> + for (i = 0; i < AW9610X_CHANNEL_MAX; i++) {
>> + aw_sar_i2c_read(p_sar->i2c,
>> + REG_AFECFG1_CH0 + i * AW_CL1SPE_CALI_OS, ®_val);
>> + coff_data = (reg_val >> 24) * 900 + ((reg_val >> 16) & 0xff) * 13;
>> + coff_data_int = coff_data / 1000;
>> + coff_data_dec = coff_data % 1000;
>> + snprintf(temp_data, sizeof(temp_data), "%u.%u", coff_data_int, coff_data_dec);
>> + len += snprintf(buf+len, PAGE_SIZE-len, "PARASITIC_DATA_CH%d = %s pf\n",
>> + i, temp_data);
>> + }
>> +
>> + return len;
>> +}
>> +
>> +static const struct aw_sar_offset_t g_aw9610x_offset = {
>> + .p_get_offset_node_fn = aw9610x_get_self_cap_offset,
>> +};
>>
>> +
>> +static void aw9610x_datablock_load(struct aw_sar *p_sar, const char *buf)
>> +{
>> + struct aw9610x *aw9610x = p_sar->priv_data;
>> + unsigned char addr_bytes = aw9610x->aw_i2c_package.addr_bytes;
>> + unsigned char data_bytes = aw9610x->aw_i2c_package.data_bytes;
>> + unsigned char reg_num = aw9610x->aw_i2c_package.reg_num;
>> + unsigned char reg_data[220] = { 0 };
>> + unsigned int databuf[220] = { 0 };
>Too big for the stack.
>
The v4 version patch will delete related code.
>> + unsigned char temp_buf[2] = { 0 };
>> + unsigned int i;
>> +
>> + for (i = 0; i < data_bytes * reg_num; i++) {
>> + if (reg_num < attr_buf[1]) {
>> + temp_buf[0] = buf[attr_buf[0] + (addr_bytes + i) * 5];
>> + temp_buf[1] =
>> + buf[attr_buf[0] + (addr_bytes + i) * 5 + 1];
>> + } else if (reg_num >= attr_buf[1] && reg_num < attr_buf[3]) {
>> + temp_buf[0] = buf[attr_buf[2] + (addr_bytes + i) * 5];
>> + temp_buf[1] =
>> + buf[attr_buf[2] + (addr_bytes + i) * 5 + 1];
>> + } else if (reg_num >= attr_buf[3] && reg_num < attr_buf[5]) {
>> + temp_buf[0] = buf[attr_buf[4] + (addr_bytes + i) * 5];
>> + temp_buf[1] =
>> + buf[attr_buf[4] + (addr_bytes + i) * 5 + 1];
>> + }
>> + sscanf(temp_buf, "%02x", &databuf[i]);
>> + reg_data[i] = (unsigned char)databuf[i];
>> + }
>> + aw9610x->aw_i2c_package.p_reg_data = reg_data;
>> + aw9610x_awrw_write_seq(p_sar);
>> +}
>> +
>> +static int aw9610x_awrw_read_seq(struct aw_sar *p_sar, unsigned char *reg_data)
>> +{
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned char data_bytes = aw9610x->aw_i2c_package.data_bytes;
>> + unsigned char addr_bytes = aw9610x->aw_i2c_package.addr_bytes;
>> + unsigned char reg_num = aw9610x->aw_i2c_package.reg_num;
>> + unsigned short msg_cnt = (unsigned short)(data_bytes * reg_num);
>> + unsigned char w_buf[4];
>> + unsigned char buf[228];
>> + unsigned int msg_idx;
>> + int ret;
>> +
>> + for (msg_idx = 0; msg_idx < addr_bytes; msg_idx++)
>> + w_buf[msg_idx] = aw9610x->aw_i2c_package.init_addr[msg_idx];
>> +
>> + ret = aw_sar_i2c_read_seq(p_sar->i2c, w_buf, 2, (unsigned char *)buf, msg_cnt);
>
>If there was an error, don't update the data passed out of here as it's
>garbage.
>
The v4 version patch will delete related code.
>> +
>> + for (msg_idx = 0; msg_idx < msg_cnt; msg_idx++)
>> + reg_data[msg_idx] = buf[msg_idx];
>> +
>> + return ret;
>> +}
>
>> +
>> +static void aw9610x_power_on_prox_detection(void *data, unsigned char en_flag)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + struct aw9610x *aw9610x = (struct aw9610x *)p_sar->priv_data;
>> + unsigned char ch;
>> +
>> + if (en_flag == true) {
>
>if (en_flag) {
>
The v4 version patch will delete related code.
>> + for (ch = 0; ch < AW9610X_CHANNEL_MAX; ch++) {
>> + aw_sar_i2c_read(p_sar->i2c,
>> + REG_BLFILT_CH0 + (REG_BLFILT_CH1 - REG_BLFILT_CH0) * ch,
>> + &(aw9610x->last_blfilta[ch]));
>> + aw_sar_i2c_write_bits(p_sar->i2c,
>> + REG_BLFILT_CH0 + (REG_BLFILT_CH1 - REG_BLFILT_CH0) * ch,
>> + ~(0x3f << 13), (1 << 13));
>> + }
>> + aw_sar_i2c_read(p_sar->i2c, REG_IRQEN, &aw9610x->last_irq_en);
>> + aw_sar_i2c_write_bits(p_sar->i2c, REG_IRQEN, ~(1 << 3), 1 << 3);
>> + } else if (en_flag == false) {
>
>} else {
>
The v4 version patch will delete related code.
>> + for (ch = 0; ch < AW9610X_CHANNEL_MAX; ch++) {
>> + aw_sar_i2c_write(p_sar->i2c,
>> + REG_BLFILT_CH0 + (REG_BLFILT_CH1 - REG_BLFILT_CH0) * ch,
>> + aw9610x->last_blfilta[ch]);
>> + }
>> + aw_sar_i2c_write(p_sar->i2c, REG_IRQEN, aw9610x->last_irq_en);
>> + }
>> +}
>> +
>> +static const struct aw_sar_power_on_prox_detection_t g_aw9610x_power_on_prox_detection = {
>> + .p_power_on_prox_detection_en_fn = aw9610x_power_on_prox_detection,
>> + .irq_en_cali_bit = 3,
>> + .power_on_prox_en_flag = true,
>> +};
>
>Why not embed this structure directly in aw_sar_chip_info?
>Seems like a number of the other structures would also be better handled that way
>
The v4 version patch will delete related code.
>> +
>> +static const struct aw_sar_chip_config g_aw9610x_chip_config = {
>> + .ch_num_max = AW9610X_CHANNEL_MAX,
>> +
>> + .p_platform_config = &g_aw9610x_platform_config,
>> +
>> + .p_check_chipid = &g_aw9610x_check_chipid,
>> + .p_soft_rst = &g_aw9610x_soft_rst,
>> + .p_init_over_irq = &g_aw9610x_init_over_irq,
>> + .p_reg_bin = &g_aw9610x_load_reg_bin,
>> + .p_chip_mode = &g_aw9610x_chip_mode,
>> +
>> + /* Node usage parameters */
>> + .p_reg_list = &g_aw9610x_reg_list,
>> + .p_reg_arr = &g_aw9610x_reg_arr_para,
>> + .p_aot = &g_aw9610x_aot,
>> + .p_diff = &g_aw9610x_diff,
>> + .p_offset = &g_aw9610x_offset,
>> + .p_mode = &g_aw9610x_mode,
>> + .p_get_chip_info = &g_aw9610x_get_chip_info,
>> + .p_aw_sar_awrw = &g_aw9610x_awrw,
>> +
>> + .p_other_operation = aw9610x_get_chip_version,
>> + .p_other_opera_free = NULL,
>> + .power_on_prox_detection = &g_aw9610x_power_on_prox_detection,
>> +};
>> +
>> +int aw9610x_init(struct aw_sar *p_sar)
>> +{
>> + if (!p_sar)
>> + return -EINVAL;
>> +
>> + g_aw_sar = p_sar;
>> +
>> + p_sar->priv_data = devm_kzalloc(p_sar->dev, sizeof(struct aw9610x), GFP_KERNEL);
>> + if (!p_sar->priv_data)
>> + return -ENOMEM;
>> +
>> + /* Chip private function operation */
>> + p_sar->p_sar_para = &g_aw9610x_chip_config;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(aw9610x_init, AWINIC_PROX);
>> +
>> +void aw9610x_deinit(struct aw_sar *p_sar)
>> +{
>> + if (p_sar->priv_data != NULL)
>> + devm_kfree(p_sar->dev, p_sar->priv_data);
>
>I haven't checked but dev_kfree() calls that aren't clearly an error path cleanup
>are almost always a bug.. The whole point is that they happen automatically and
>you don't need to call them.
>
The v4 version patch will delete related code.
>> +}
>> +EXPORT_SYMBOL_NS_GPL(aw9610x_deinit, AWINIC_PROX);
>> +
...
>> diff --git a/drivers/iio/proximity/aw_sar_comm_interface.c b/drivers/iio/proximity/aw_sar_comm_interface.c
>> new file mode 100644
>> index 000000000000..8a7d437d100b
>> --- /dev/null
>> +++ b/drivers/iio/proximity/aw_sar_comm_interface.c
>> @@ -0,0 +1,550 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include "aw_sar_comm_interface.h"
>> +
>> +#define AW_I2C_RW_RETRY_TIME_MIN (2000)
>> +#define AW_I2C_RW_RETRY_TIME_MAX (3000)
>> +#define AW_RETRIES (5)
>> +
>> +static int awinic_i2c_write(struct i2c_client *i2c, unsigned char *tr_data, unsigned short len)
>> +{
>> + struct i2c_msg msg;
>> +
>> + msg.addr = i2c->addr;
>> + msg.flags = 0;
>> + msg.len = len;
>> + msg.buf = tr_data;
>> +
>> + return i2c_transfer(i2c->adapter, &msg, 1);
>
>Looks like i2c_master_send()
>
>Use that inline rather than having your own version.
>
In the next version of the patch, I will use regmap.
>> +}
>> +
>> +static int awinic_i2c_read(struct i2c_client *i2c, unsigned char *addr,
>> + unsigned char addr_len, unsigned char *data, unsigned short data_len)
>Why would a length be a char? u8 maybe.
>Also data as u8 not char.
The patch for v4 will fix these issues.
>> +{
>> + struct i2c_msg msg[2];
>> +
>> + msg[0].addr = i2c->addr;
>> + msg[0].flags = 0;
>> + msg[0].len = addr_len;
>> + msg[0].buf = addr;
>> +
>> + msg[1].addr = i2c->addr;
>> + msg[1].flags = 1;
>> + msg[1].len = data_len;
>> + msg[1].buf = data;
>> +
>> + return i2c_transfer(i2c->adapter, msg, 2);
>> +}
>> +
>> +int aw_sar_i2c_read(struct i2c_client *i2c, unsigned short reg_addr16, unsigned int *reg_data32)
>> +{
>> + unsigned char r_buf[6] = { 0 };
>> + unsigned char cnt = 5;
>> + int ret;
>> +
>> + if (!i2c)
>> + return -EINVAL;
>> +
>> + r_buf[0] = (unsigned char)(reg_addr16 >> OFFSET_BIT_8);
>> + r_buf[1] = (unsigned char)(reg_addr16);
>
>put_unaligned_be16()
The v4 version patch will delete related code.
>
>> +
>> + do {
>> + ret = awinic_i2c_read(i2c, r_buf, 2, &r_buf[2], 4);
>Use two buffers as easeir to read and you can type them correctly as __be16 and
>__be32 I think.
>
>> + if (ret < 0)
>> + dev_err(&i2c->dev, "i2c read error reg: 0x%04x, ret= %d cnt= %d",
>> + reg_addr16, ret, cnt);
>> + else
>> + break;
>> + usleep_range(2000, 3000);
>
>Any retry needs documentation on why it might be necessary + why these values.
>
The v4 version patch will delete related code.
>> + } while (cnt--);
>> +
>> + if (cnt < 0) {
>> + dev_err(&i2c->dev, "i2c read error!");
>> + return ret;
>> + }
>> +
>> + *reg_data32 = ((unsigned int)r_buf[5] << OFFSET_BIT_0) | ((unsigned int)r_buf[4] << OFFSET_BIT_8) |
>> + ((unsigned int)r_buf[3] << OFFSET_BIT_16) | ((unsigned int)r_buf[2] << OFFSET_BIT_24);
>get_unaligned_be32()
>or be32_to_cpu() if you use a __be32 as the buffer in the first place
>> +
>> + return 0;
>> +}
>> +
>> +int aw_sar_i2c_write(struct i2c_client *i2c, unsigned short reg_addr16, unsigned int reg_data32)
>
>u16 and u32 to get kernel fixed width types.
>
>
The patch for v4 will fix these issues.
>> +{
>> + unsigned char w_buf[6] = { 0 };
>You fill it all so don't initialize.
>
The patch for v4 will fix these issues.
>> + unsigned char cnt = 5;
>> + int ret;
>> +
>> + if (!i2c)
>> + return -EINVAL;
>> +
>> + /* reg_addr */
>> + w_buf[0] = (unsigned char)(reg_addr16 >> OFFSET_BIT_8);
>> + w_buf[1] = (unsigned char)(reg_addr16);
>
>put_unaligned_be16()
>
The patch for v4 will fix these issues.
>> + /* data */
>> + w_buf[2] = (unsigned char)(reg_data32 >> OFFSET_BIT_24);
>> + w_buf[3] = (unsigned char)(reg_data32 >> OFFSET_BIT_16);
>> + w_buf[4] = (unsigned char)(reg_data32 >> OFFSET_BIT_8);
>> + w_buf[5] = (unsigned char)(reg_data32);
>put_unaligned_be32() or similar.
>
The patch for v4 will fix these issues.
>> +
>> + do {
>> + ret = awinic_i2c_write(i2c, w_buf, ARRAY_SIZE(w_buf));
>> + if (ret < 0) {
>> + dev_err(&i2c->dev,
>> + "i2c write error reg: 0x%04x data: 0x%08x, ret= %d cnt= %d",
>> + reg_addr16, reg_data32, ret, cnt);
>> + } else {
>> + break;
>> + }
>> + } while (cnt--);
>Why are retries needed? Very unlikely that they are on a sensible circuit board.
>Prefer to just report an error to the caller if one occurs rather than try again.
>
The v4 version patch will delete related code.
>> +
>> + if (cnt < 0) {
>> + dev_err(&i2c->dev, "i2c write error!");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int
>> +aw_sar_i2c_write_bits(struct i2c_client *i2c, unsigned short reg_addr16, unsigned int mask, unsigned int val)
>> +{
>> + unsigned int reg_val;
>> +
>> + aw_sar_i2c_read(i2c, reg_addr16, ®_val);
>> + reg_val &= mask;
>> + reg_val |= (val & (~mask));
>> + aw_sar_i2c_write(i2c, reg_addr16, reg_val);
>
>May well makes sense to use a custom regmap as then you will get all this stuff for
>free.
>
In the next version of the patch, I will use regmap.
>> +
>> + return 0;
>> +}
>> +
>> +int aw_sar_i2c_write_seq(struct i2c_client *i2c, unsigned char *tr_data, unsigned short len)
>> +{
>> + unsigned char cnt = AW_RETRIES;
>> + int ret;
>> +
>> + do {
>> + ret = awinic_i2c_write(i2c, tr_data, len);
>> + if (ret < 0)
>> + dev_err(&i2c->dev, "awinic i2c write seq error %d", ret);
>> + else
>> + break;
>> + usleep_range(AW_I2C_RW_RETRY_TIME_MIN, AW_I2C_RW_RETRY_TIME_MAX);
>> + } while (cnt--);
>
>As above. Why retries?
>
The v4 version patch will delete related code.
>> +
>> + if (cnt < 0) {
>> + dev_err(&i2c->dev, "awinic i2c write error!");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int aw_sar_i2c_read_seq(struct i2c_client *i2c, unsigned char *addr,
>> + unsigned char addr_len, unsigned char *data, unsigned short data_len)
>> +{
>> + unsigned char cnt = AW_RETRIES;
>> + int ret;
>> +
>> + do {
>> + ret = awinic_i2c_read(i2c, addr, addr_len, data, data_len);
>> + if (ret < 0)
>> + dev_err(&i2c->dev, "awinic sar i2c write error %d", ret);
>> + else
>> + break;
>> + usleep_range(AW_I2C_RW_RETRY_TIME_MIN, AW_I2C_RW_RETRY_TIME_MAX);
>> + } while (cnt--);
>> +
>> + if (cnt < 0) {
>> + dev_err(&i2c->dev, "awinic sar i2c read error!");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +enum bin_header_version_enum {
>> + HEADER_VERSION_1_0_0 = 0x01000000,
>> +};
>> +
>> +enum data_type_enum {
>> + DATA_TYPE_REGISTER = 0x00000000,
>> + DATA_TYPE_DSP_REG = 0x00000010,
>> + DATA_TYPE_DSP_CFG = 0x00000011,
>> + DATA_TYPE_SOC_REG = 0x00000020,
>> + DATA_TYPE_SOC_APP = 0x00000021,
>> + DATA_TYPE_MULTI_BINS = 0x00002000,
>> +};
>> +
>> +#define BigLittleSwap16(A) ((((unsigned short int)(A) & 0xff00) >> 8) | \
>> + (((unsigned short int)(A) & 0x00ff) << 8))
>Why? We have standard functions for this swab16()
>
>> +
>> +#define BigLittleSwap32(A) ((((unsigned long)(A) & 0xff000000) >> 24) | \
>> + (((unsigned long)(A) & 0x00ff0000) >> 8) | \
>> + (((unsigned long)(A) & 0x0000ff00) << 8) | \
>> + (((unsigned long)(A) & 0x000000ff) << 24))
>swab32()
>
The v4 version patch will delete related code.
>> +
>> +static enum aw_bin_err_val aw_parse_bin_header_1_0_0(struct aw_bin *bin);
>> +
>> +static enum aw_bin_err_val aw_check_sum(struct aw_bin *bin, int bin_num)
>> +{
>> + unsigned char *p_check_sum;
>> + unsigned int sum_data = 0;
>> + unsigned int check_sum;
>> + unsigned int i;
>> +
>> + p_check_sum = &(bin->info.data[(bin->header_info[bin_num].valid_data_addr -
>> + bin->header_info[bin_num].header_len)]);
>> + check_sum = AW_SAR_GET_32_DATA(*(p_check_sum + 3), *(p_check_sum + 2),
>> + *(p_check_sum + 1), *(p_check_sum));
>> +
>> + for (i = 4; i < bin->header_info[bin_num].bin_data_len +
...
>> +
>> diff --git a/drivers/iio/proximity/aw_sar_comm_interface.h b/drivers/iio/proximity/aw_sar_comm_interface.h
>> new file mode 100644
>> index 000000000000..bc543bceb17d
>> --- /dev/null
>> +++ b/drivers/iio/proximity/aw_sar_comm_interface.h
>> @@ -0,0 +1,172 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef _AW_SAR_PLAT_HW_INTERFACE_H_
>> +#define _AW_SAR_PLAT_HW_INTERFACE_H_
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +
>> +enum aw_sar_chip_list_t {
>> + AW_SAR_NONE_CHECK_CHIP,
>> +
>> + SAR_AW9610X = 1 << 1,
>> + SAR_AW9610XA = 1 << 2,
>> +
>> + SAR_AW96303 = 1 << 6,
>> + SAR_AW96305 = 1 << 7,
>> + SAR_AW96308 = 1 << 8,
>> + SAR_AW96310 = 1 << 9,
>> + SAR_AW96312 = 1 << 10,
>> +};
>> +
>> +#ifndef AW_TRUE
>> +#define AW_TRUE (1)
>> +#endif
>> +
>> +#ifndef AW_FALSE
>> +#define AW_FALSE (0)
>> +#endif
>Just use true and false rather than adding your own definitions.
>
The v4 version patch will delete related code.
>> +
>> +#define AW_ERR_IRQ_INIT_OVER (0xAA)
>> +
>> +enum aw_sar_rst_val {
>> + AW_OK,
>> + AW_BIN_PARA_INVALID,
>> + AW_PROT_UPDATE_ERR,
>> + AW_REG_LOAD_ERR,
>> +};
>> +
>> +#ifndef OFFSET_BIT_0
>> +#define OFFSET_BIT_0 (0)
>> +#endif
>> +
>> +#ifndef OFFSET_BIT_8
>> +#define OFFSET_BIT_8 (8)
>> +#endif
>> +
>> +#ifndef OFFSET_BIT_16
>> +#define OFFSET_BIT_16 (16)
>> +#endif
>> +
>> +#ifndef OFFSET_BIT_24
>> +#define OFFSET_BIT_24 (24)
>> +#endif
>> +
>> +#define AW_SAR_GET_32_DATA(w, x, y, z) ((unsigned int)(((w) << 24) | ((x) << 16) | ((y) << 8) | (z)))
>> +
>> +enum AW_SAR_HOST_IRQ_STAT {
>> + IRQ_ENABLE,
>> + IRQ_DISABLE,
>> +};
>> +
>> +#define AW_SAR_BIN_NUM_MAX 100
>> +
>> +enum aw_bin_err_val {
>> + AW_BIN_ERROR_NONE = 0,
>> + AW_BIN_ERROR_HEADER_VERSION = -1,
>> + AW_BIN_ERROR_DATA_TYPE = -2,
>> + AW_BIN_ERROR_SUM_OR_DATA_LEN = -3,
>> + AW_BIN_ERROR_DATA_VERSION = -4,
>> + AW_BIN_ERROR_REGISTER_NUM = -5,
>> + AW_BIN_ERROR_DSP_REG_NUM = -6,
>> + AW_BIN_ERROR_SOC_APP_NUM = -7,
>> + AW_BIN_ERROR_NULL_POINT = -8,
>> +};
>> +
>> +/**
>> + * struct bin_header_info -
>> + * @header_len: Frame header length
>> + * @check_sum: Frame header information-Checksum
>> + * @header_ver: Frame header information-Frame header version
>> + * @bin_data_type: Frame header information-Data type
>> + * @bin_data_ver: Frame header information-Data version
>> + * @bin_data_len: Frame header information-Data length
>> + * @ui_ver: Frame header information-ui version
>> + * @chip_type: Frame header information-chip type
>> + * @reg_byte_len: Frame header information-reg byte len
>> + * @data_byte_len: Frame header information-data byte len
>> + * @device_addr: Frame header information-device addr
>> + * @valid_data_len: Length of valid data obtained after parsing
>> + * @valid_data_addr: The offset address of the valid data obtained
>> + * after parsing relative to info
>> + * @reg_num: The number of registers obtained after parsing
>> + * @reg_data_byte_len: The byte length of the register obtained after parsing
>> + * @download_addr: The starting address or download address obtained after parsing
>> + * @app_version: The software version number obtained after parsing
>> + */
>> +struct bin_header_info {
>> + unsigned int header_len;
>> + unsigned int check_sum;
>> + unsigned int header_ver;
>> + unsigned int bin_data_type;
>> + unsigned int bin_data_ver;
>> + unsigned int bin_data_len;
>> + unsigned int ui_ver;
>> + unsigned char chip_type[8];
>> + unsigned int reg_byte_len;
>> + unsigned int data_byte_len;
>> + unsigned int device_addr;
>> + unsigned int valid_data_len;
>> + unsigned int valid_data_addr;
>> + unsigned int reg_num;
>> + unsigned int reg_data_byte_len;
>> + unsigned int download_addr;
>> + unsigned int app_version;
>> +};
>> +
>> +/**
>> + * struct bin_container -
>> + * @len: The size of the bin file obtained from the firmware
>> + * @data: Store the bin file obtained from the firmware
>> + */
>> +struct bin_container {
>> + unsigned int len;
>> + unsigned char data[];
>> +};
>> +
>> +/**
>> + * struct aw_bin -
>> + * @p_addr: Offset pointer (backward offset pointer to obtain frame header information and
>> + * important information)
>> + * @all_bin_parse_num: The number of all bin files
>> + * @multi_bin_parse_num: The number of single bin files
>> + * @single_bin_parse_num: The number of multiple bin files
>> + * @header_info: Frame header information and other important data obtained after parsing
>> + * @info: Obtained bin file data that needs to be parsed
>> + */
>> +struct aw_bin {
>> + char *p_addr;
>> + unsigned int all_bin_parse_num;
>> + unsigned int multi_bin_parse_num;
>> + unsigned int single_bin_parse_num;
>> + struct bin_header_info header_info[AW_SAR_BIN_NUM_MAX];
>> + struct bin_container info;
>> +};
>> +
>> +/* I2C communication API */
>> +int aw_sar_i2c_read(struct i2c_client *i2c, unsigned short reg_addr16, unsigned int *reg_data32);
>> +int aw_sar_i2c_write(struct i2c_client *i2c, unsigned short reg_addr16, unsigned int reg_data32);
>> +int aw_sar_i2c_write_bits(struct i2c_client *i2c, unsigned short reg_addr16,
>> + unsigned int mask, unsigned int val);
>> +int aw_sar_i2c_write_seq(struct i2c_client *i2c, unsigned char *tr_data, unsigned short len);
>> +int aw_sar_i2c_read_seq(struct i2c_client *i2c, unsigned char *addr,
>> + unsigned char addr_len, unsigned char *data, unsigned short data_len);
>> +
>> +enum aw_bin_err_val aw_sar_parsing_bin_file(struct aw_bin *bin);
>> +unsigned int aw_sar_pow2(unsigned int cnt);
>> +int aw_sar_load_reg(struct aw_bin *aw_bin, struct i2c_client *i2c);
>> +
>> +#endif
>> diff --git a/drivers/iio/proximity/aw_sar_type.h b/drivers/iio/proximity/aw_sar_type.h
>> new file mode 100644
>> index 000000000000..e6744855315c
>> --- /dev/null
>> +++ b/drivers/iio/proximity/aw_sar_type.h
>> @@ -0,0 +1,371 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef _SAR_TYPE_H_
>> +#define _SAR_TYPE_H_
>> +
>> +#include "aw_sar_comm_interface.h"
>> +
>> +typedef int (*aw_sar_chip_other_operation_t)(void *data);
>> +typedef void (*aw_sar_chip_other_opera_free_t)(void *data);
>> +
>> +enum aw_i2c_flags {
>> + AW_SAR_I2C_WR,
>> + AW_SAR_I2C_RD,
>> + AW_SAR_PACKAGE_RD,
>> +};
>> +
>> +enum sar_health_check {
>> + AW_SAR_HEALTHY = 0,
>> + AW_SAR_UNHEALTHY = 1,
>> +};
>> +typedef int (*aw_sar_bin_opera_t)(struct aw_bin *aw_bin, void *load_bin_para);
>> +typedef int (*aw_sar_bin_load_fail_opera_t)(struct aw_bin *aw_bin, void *load_bin_para);
>> +
>> +struct aw_sar_get_chip_info_t {
>> + void (*p_get_chip_info_node_fn)(void *data, char *buf, ssize_t *p_len);
>> +};
>> +
>> +struct aw_sar_load_bin_t {
>> + const unsigned char *bin_name;
>> + aw_sar_bin_opera_t bin_opera_func;
>> + aw_sar_bin_load_fail_opera_t bin_load_fail_opera;
>> +
>> + void (*p_get_prot_update_fw_node_fn)(void *data, char *buf, ssize_t *p_len);
>> +
>> + /* Perform different operations to update parameters */
>> + int (*p_update_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_reg_data {
>> + unsigned char rw;
>> + unsigned short reg;
>> +};
>> +
>> +struct aw_sar_awrw_t {
>> + ssize_t (*p_set_awrw_node_fn)(void *data, const char *buf, size_t count);
>> + ssize_t (*p_get_awrw_node_fn)(void *data, char *buf);
>> +};
>> +
>> +struct aw_sar_reg_list_t {
>> + unsigned char reg_none_access;
>> + unsigned char reg_rd_access;
>> + unsigned char reg_wd_access;
>> + const struct aw_sar_reg_data *reg_perm;
>> + unsigned int reg_num;
>> +};
>> +
>> +typedef void (*aw_sar_update_work_t)(struct work_struct *work);
>> +struct aw_sar_update_static_t {
>> + aw_sar_update_work_t update_work_func;
>> + unsigned int delay_ms;
>> +};
>> +
>> +typedef irqreturn_t (*aw_sar_irq_t)(int irq, void *data);
>> +typedef unsigned int (*sar_rc_irqscr_t)(void *i2c);
>> +/*
>> + * If the return value is 1, there is an initialization completion interrupt;
>> + * if the return value is 0, there is no
>> + */
>> +typedef unsigned int (*aw_sar_is_init_over_irq)(unsigned int irq_status);
>> +typedef void (*aw_sar_irq_spec_handler_t)(unsigned int irq_status, void *data);
>> +
>> +struct aw_sar_check_chipid_t {
>> + /* Read chipid and check chipid, Must be implemented externally */
>> + int (*p_check_chipid_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_irq_init_t {
>> + unsigned long flags;
>> + unsigned long irq_flags;
>> + irq_handler_t handler;
>> + irq_handler_t thread_fn;
>> + /* Interrupt processing parameters */
>> + sar_rc_irqscr_t rc_irq_fn;
>> + /* aw_sar_is_init_over_irq is_init_over_irq_fn; */
>> + aw_sar_irq_spec_handler_t irq_spec_handler_fn;
>> +
>> + /* Use a different initialization interrupt to initialize the operation */
>> + int (*p_irq_init_fn)(void *data);
>> + /* Release interrupt resource */
>> + int (*p_irq_deinit_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_pm_t {
>> + unsigned int suspend_set_mode;
>> + unsigned int resume_set_mode;
>> + unsigned int shutdown_set_mode;
>> + /* system api */
>> + int (*p_suspend_fn)(void *data);
>> + int (*p_resume_fn)(void *data);
>> + int (*p_shutdown_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_chip_mode_t {
>> + unsigned int init_mode;
>> + unsigned int active;
>> + unsigned int pre_init_mode;
>> +};
>> +
>> +struct aw_sar_regulator_config_t {
>> + /* Note that "_sar_num" after VCC name is defined by SAR C auto add */
>> + const unsigned char *vcc_name;
>> + int min_uV;
>> + int max_uV;
>> +};
>> +
>> +struct aw_channels_info {
>> + unsigned short used;
>> + unsigned int last_channel_info;
>> +};
>> +
>> +struct aw_sar_dts_info {
>> + unsigned int sar_num;
>> + unsigned int channel_use_flag;
>> + bool use_regulator_flag;
>> + bool use_inter_pull_up;
>> + bool use_pm;
>> + bool use_plug_cail_flag;
>> + bool monitor_esd_flag;
>> +};
>> +
>> +struct aw_sar_irq_init_comm_t {
>> + unsigned char host_irq_stat;
>> + void *data;
>> + unsigned char dev_id[30];
>> +};
>> +
>> +struct aw_sar_load_bin_comm_t {
>> + unsigned char bin_name[30];
>> + unsigned int bin_data_ver;
>> + aw_sar_bin_opera_t bin_opera_func;
>> + aw_sar_bin_load_fail_opera_t bin_load_fail_opera_func;
>> +};
>> +
>> +struct aw_awrw_info {
>> + unsigned char rw_flag;
>> + unsigned char addr_len;
>> + unsigned char data_len;
>> + unsigned char reg_num;
>> + unsigned int i2c_tranfar_data_len;
>> + unsigned char *p_i2c_tranfar_data;
>> +};
>> +
>> +typedef void (*sar_enable_clock_t)(void *i2c);
>> +typedef void (*sar_operation_irq_t)(int to_irq);
>> +typedef void (*sar_mode_update_t)(void *i2c);
>> +
>> +struct aw_sar_mode_switch_ops {
>> + sar_enable_clock_t enable_clock;
>
>Just use the type directly. The typdef doesn't help with readability.
>
The v4 version patch will delete related code.
>> + sar_rc_irqscr_t rc_irqscr;
>> + sar_mode_update_t mode_update;
>> +};
>> +
>> +struct aw_sar_chip_mode {
>> + unsigned char curr_mode;
>> + unsigned char last_mode;
>> +};
>> +
>> +struct aw_sar_mode_set_t {
>> + unsigned char chip_id;
>> + struct aw_sar_chip_mode chip_mode;
>> + struct aw_sar_mode_switch_ops mode_switch_ops;
>> +};
>> +
>> +struct aw_sar_mode_t {
>> + const struct aw_sar_mode_set_t *mode_set_arr;
>> + unsigned char mode_set_arr_len;
>> + ssize_t (*p_set_mode_node_fn)(void *data, unsigned char curr_mode);
>> + ssize_t (*p_get_mode_node_fn)(void *data, char *buf);
>> +};
>> +
>> +struct aw_sar_init_over_irq_t {
>> + short wait_times;
>> + unsigned char daley_step;
>> + unsigned int reg_irqsrc;
>> + unsigned int irq_offset_bit;
>> + unsigned int irq_mask;
>> + unsigned int irq_flag;
>> + /*
>> + * Perform different verification initialization
>> + * to complete the interrupt operation
>> + */
>> + int (*p_check_init_over_irq_fn)(void *data);
>> + /*
>> + * When initialization fails, get the corresponding error type and
>> + * apply it to the chip with flash
>> + */
>> + int (*p_get_err_type_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_soft_rst_t {
>> + unsigned short reg_rst;
>> + unsigned int reg_rst_val;
>> + unsigned int delay_ms;
>> + /* Perform different soft reset operations */
>> + int (*p_soft_reset_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_aot_t {
>> + unsigned int aot_reg;
>> + unsigned int aot_mask;
>> + unsigned int aot_flag;
>> + ssize_t (*p_set_aot_node_fn)(void *data);
>> +};
>> +
>> +struct aw_sar_diff_t {
>> + unsigned short diff0_reg;
>> + unsigned short diff_step;
>> + /* Data format:S21.10, Floating point types generally need to be removed */
>> + unsigned int rm_float;
>> + ssize_t (*p_get_diff_node_fn)(void *data, char *buf);
>> +};
>> +
>> +struct aw_sar_offset_t {
>> + ssize_t (*p_get_offset_node_fn)(void *data, char *buf);
>> +};
>> +
>> +struct aw_sar_pinctrl {
>> + struct pinctrl *pinctrl;
>> + struct pinctrl_state *default_sta;
>> + struct pinctrl_state *int_out_high;
>> + struct pinctrl_state *int_out_low;
>> +};
>> +
>> +/* update reg node */
>> +struct aw_sar_para_load_t {
>> + const unsigned int *reg_arr;
>> + unsigned int reg_arr_len;
>> +};
>> +
>> +struct aw_sar_platform_config {
>> + /* The chip needs to parse more DTS contents for addition */
>> + int (*p_add_parse_dts_fn)(void *data);
>> +
>> + const struct aw_sar_regulator_config_t *p_regulator_config;
>> +
>> + /* The chip needs to add more nodes */
>> + int (*p_add_node_create_fn)(void *data);
>> + /* Release the added node */
>> + int (*p_add_node_free_fn)(void *data);
>> +
>> + /* Use a different initialization interrupt to initialize the operation */
>> + int (*p_input_init_fn)(void *data);
>> + /* Release input resource */
>> + int (*p_input_deinit_fn)(void *data);
>> +
>> + /* The parameters passed in are required for interrupt initialization */
>> + const struct aw_sar_irq_init_t *p_irq_init;
>> +
>> + /* The chip is set to different modes in different power management interfaces */
>> + const struct aw_sar_pm_t *p_pm_chip_mode;
>> +};
>> +
>> +struct aw_sar_power_on_prox_detection_t {
>> + /* en_flag is true enable */
>> + void (*p_power_on_prox_detection_en_fn)(void *data, unsigned char en_flag);
>> + unsigned int irq_en_cali_bit;
>> + unsigned char power_on_prox_en_flag;
>> +};
>> +
>> +/**
>> + * struct aw_sar_chip_config -
>A lot of this is not 'config' as such. It's per chip type constant data.
>As such better to name it to reflect that. chip_info is typical choice.
>
The v4 version patch will delete related code.
>> + * @ch_num_max: Number of channels of the chip
>> + * @p_platform_config: Chip related platform content configuration
>> + * @p_check_chipid: Parameters required for verification of chipid
>> + * @p_soft_rst: Parameters required for soft reset
>> + * @p_init_over_irq: Verify the parameters required to initialize a complete interrupt
>> + * @p_reg_bin: Parameters required for load register bin file
>> + * @p_chip_mode: The mode set before and after the initialization of the chip
>> + * @p_reg_list: Register permission table
>> + * @p_reg_arr: Default register table
>> + * @p_aot: Parameters required for set Auto-Offset-Tuning(aot)
>> + * @p_diff: Parameters required for get chip diff val
>> + * @p_offset: Parameters required for get chip offset val
>> + * @p_mode: Set the parameters of different working modes of the chip
>> + * @p_get_chip_info: Obtain the necessary information of the chip
>> + * @p_aw_sar_awrw: Continuous read/write register interface
>> + * @p_other_operation: Other operations during initialization, Add according to different usage
>> + * @p_other_opera_free: If requested by resources, please release
>> + */
>> +struct aw_sar_chip_config {
>> + unsigned char ch_num_max;
>> + const struct aw_sar_platform_config *p_platform_config;
>> + const struct aw_sar_check_chipid_t *p_check_chipid;
>> + const struct aw_sar_soft_rst_t *p_soft_rst;
>don't do the _t thing. It's obviously a type because it's a struct.
>
The v4 version patch will delete related code.
>> + const struct aw_sar_init_over_irq_t *p_init_over_irq;
>> + const struct aw_sar_load_bin_t *p_reg_bin;
>> + const struct aw_sar_chip_mode_t *p_chip_mode;
>> + const struct aw_sar_reg_list_t *p_reg_list;
>> + const struct aw_sar_para_load_t *p_reg_arr;
>> + const struct aw_sar_aot_t *p_aot;
>> + const struct aw_sar_diff_t *p_diff;
>> + const struct aw_sar_offset_t *p_offset;
>> + const struct aw_sar_mode_t *p_mode;
>> + const struct aw_sar_get_chip_info_t *p_get_chip_info;
>> + const struct aw_sar_awrw_t *p_aw_sar_awrw;
>> + aw_sar_chip_other_operation_t p_other_operation;
>> + aw_sar_chip_other_opera_free_t p_other_opera_free;
>> + const struct aw_sar_power_on_prox_detection_t *power_on_prox_detection;
>> +};
>> +
>> +struct aw_sar {
>> + struct i2c_client *i2c;
>> + struct device *dev;
>> + struct regulator *vcc;
>> + struct delayed_work update_work;
>> + /* Set pin pull-up mode */
>> + struct aw_sar_pinctrl pinctrl;
>> + /* eds work */
>> + struct delayed_work monitor_work;
>> + struct workqueue_struct *monitor_wq;
>> + struct iio_dev *aw_iio_dev;
>> +
>> + unsigned char chip_type;
>> + unsigned char chip_name[20];
>> +
>> + bool power_enable;
>> + bool fw_fail_flag;
>> + unsigned char last_mode;
>> +
>> + /* handler_anomalies */
>> + unsigned char fault_flag;
>> + unsigned char driver_code_initover_flag;
>> + /* handler_anomalies */
>> +
>> + unsigned char ret_val;
>> + unsigned char curr_use_driver_type;
>> + int prot_update_state;
>> +
>> + unsigned char aot_irq_num;
>> + unsigned char enter_irq_handle_num;
>> + unsigned char exit_power_on_prox_detection;
>> +
>> + struct work_struct ps_notify_work;
>> + struct notifier_block ps_notif;
>> + bool ps_is_present;
>> +
>> + /* Parameters related to platform logic */
>> + struct aw_sar_dts_info dts_info;
>> + struct aw_sar_load_bin_comm_t load_bin;
>> + struct aw_sar_irq_init_comm_t irq_init;
>> + struct aw_awrw_info awrw_info;
>> + struct aw_channels_info *channels_arr;
>> +
>> + /* Private arguments required for public functions */
>> + const struct aw_sar_chip_config *p_sar_para;
>> + /* Private arguments required for private functions */
>> + void *priv_data;
>> +};
>> +
>> +/* Determine whether the chip exists by verifying chipid */
>> +typedef int (*aw_sar_who_am_i_t)(void *data);
>> +typedef int (*aw_sar_chip_init_t)(struct aw_sar *p_sar);
>> +typedef void (*aw_sar_chip_deinit_t)(struct aw_sar *p_sar);
>
>Just use these inline. They don't help readability enough to
>be worth typdefs.
>
The v4 version patch will delete related code.
>
>> +
>> +struct aw_sar_driver_type {
>
>Used in one place. Push it down into that file.
>> + unsigned char driver_type;
>> + aw_sar_who_am_i_t p_who_am_i;
>> + aw_sar_chip_init_t p_chip_init;
>> + aw_sar_chip_deinit_t p_chip_deinit;
>> +};
>> +
>> +#endif
Kind regards,
Wang Shuaijie
